fix static checker warnings in

PATCH Superseded Geliang Tang 4 participants 11 messages Nov 9, 2020 – Nov 12, 2020

Subsystems (3)

Networking Network drivers Networking MPTCP

lore

Share & tools

lore

Geliang Tang

author

2020-11-09 13:59 UTC #0 lore raw

cover fix static checker warnings in

This patchset fixed static checker warnings in mptcp_pm_add_timer and mptcp_pm_alloc_anno_list.

Geliang Tang (2):
mptcp: fix static checker warnings in mptcp_pm_add_timer
mptcp: cleanup for mptcp_pm_alloc_anno_list

 net/mptcp/pm_netlink.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

Signature · 1 line

2.26.2

Geliang Tang

author

2020-11-09 13:59 UTC #1 lore raw

PATCH 1/2 mptcp: fix static checker warnings in mptcp_pm_add_timer

Fix the following Smatch complaint:

net/mptcp/pm_netlink.c:213 mptcp_pm_add_timer()
warn: variable dereferenced before check 'msk' (see line 208)

net/mptcp/pm_netlink.c

    207          struct mptcp_sock *msk = entry->sock;
    208          struct sock *sk = (struct sock *)msk;
    209          struct net *net = sock_net(sk);
                                           ^^

"msk" dereferenced here.

    210
    211          pr_debug("msk=%p", msk);
    212
    213          if (!msk)
                    ^^^^

Too late.

    214                  return;
    215

Fixes 93f323b9cccc ("mptcp: add a new sysctl add_addr_timeout")

Reported-by Dan Carpenter <dan.carpenter(a)oracle.com>

Signed-off-by Geliang Tang <geliangtang(a)gmail.com>

Reviewed-by Dan Carpenter <dan.carpenter(a)oracle.com>

---
 net/mptcp/pm_netlink.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

net/mptcp/pm_netlink.c +1 -2

index 6180a8b39a3f..03f2c28f11f5 100644
@@ -206,7 +206,6 @@ static void mptcp_pm_add_timer(struct timer_list *timer)
 	struct mptcp_pm_add_entry *entry = from_timer(entry, timer, add_timer);
 	struct mptcp_sock *msk = entry->sock;
 	struct sock *sk = (struct sock *)msk;
-	struct net *net = sock_net(sk);
 
 	pr_debug("msk=%p", msk);

@@ -235,7 +234,7 @@ static void mptcp_pm_add_timer(struct timer_list *timer)
 
 	if (entry->retrans_times < ADD_ADDR_RETRANS_MAX)
 		sk_reset_timer(sk, timer,
-			       jiffies + mptcp_get_add_addr_timeout(net));
+			       jiffies + mptcp_get_add_addr_timeout(sock_net(sk)));
 
 	spin_unlock_bh(&msk->pm.lock);

Signature · 1 line

2.26.2

Geliang Tang

author

2020-11-09 13:59 UTC #2 lore raw

PATCH 2/2 mptcp: cleanup for mptcp_pm_alloc_anno_list

This patch added NULL pointer check for mptcp_pm_alloc_anno_list, and avoided similar static checker warnings in mptcp_pm_add_timer.

Signed-off-by Geliang Tang <geliangtang(a)gmail.com>

Reviewed-by Dan Carpenter <dan.carpenter(a)oracle.com>

---
 net/mptcp/pm_netlink.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

net/mptcp/pm_netlink.c +4 -2

index 03f2c28f11f5..dfc1bed4a55f 100644
@@ -266,7 +266,9 @@ static bool mptcp_pm_alloc_anno_list(struct mptcp_sock *msk,
 {
 	struct mptcp_pm_add_entry *add_entry = NULL;
 	struct sock *sk = (struct sock *)msk;
-	struct net *net = sock_net(sk);
+
+	if (!msk)
+		return false;
 
 	if (lookup_anno_list_by_saddr(msk, &entry->addr))
 		return false;

@@ -283,7 +285,7 @@ static bool mptcp_pm_alloc_anno_list(struct mptcp_sock *msk,
 
 	timer_setup(&add_entry->add_timer, mptcp_pm_add_timer, 0);
 	sk_reset_timer(sk, &add_entry->add_timer,
-		       jiffies + mptcp_get_add_addr_timeout(net));
+		       jiffies + mptcp_get_add_addr_timeout(sock_net(sk)));
 
 	return true;
 }

Signature · 1 line

2.26.2

Matthieu Baerts

2020-11-09 16:35 UTC #3 lore raw

Re: [MPTCP][PATCH net 2/2] mptcp: cleanup for mptcp_pm_alloc_anno_list

Hi Geliang,

Quoted 5 lines · Geliang Tang

This patch added NULL pointer check for mptcp_pm_alloc_anno_list, and avoided similar static checker warnings in mptcp_pm_add_timer.
Signed-off-by Geliang Tang <geliangtang(a)gmail.com>
Reviewed-by Dan Carpenter <dan.carpenter(a)oracle.com>

I think Dan reviewed the v1 of your patch -- without some modifications below -- but not the v2 nor this one.

Quoted 9 lines

---
  net/mptcp/pm_netlink.c | 6 ++++--
  1 file changed, 4 insertions(+), 2 deletions(-)

net/mptcp/pm_netlink.c +0 -0

index 03f2c28f11f5..dfc1bed4a55f 100644

net/mptcp/pm_netlink.c static bool mptcp_pm_alloc_anno_list(struct mptcp_sock *msk, · line 271

Quoted 7 lines

net/mptcp/pm_netlink.c +3 -1

@@ -266,7 +266,9 @@ static bool mptcp_pm_alloc_anno_list(struct mptcp_sock *msk,
  {
  	struct mptcp_pm_add_entry *add_entry = NULL;
  	struct sock *sk = (struct sock *)msk;
-	struct net *net = sock_net(sk);
+
+	if (!msk)
+		return false;

As Dan mentioned on MPTCP ML, this check is not required: "msk" cannot be NULL here.

We can maybe keep the cleanup (only move sock_net() below) but I don't think we need or want this in -net.
I am not even sure we want it in net-next but why not :) This could also be part of other refactors.

Cheers,
Matt

Signature · 2 lines

Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

Matthieu Baerts

Reviewed-by on v1 1/2

2020-11-09 16:28 UTC #4 lore raw

Re: [MPTCP][PATCH net 1/2] mptcp: fix static checker warnings in mptcp_pm_add_timer

In reply to Geliang Tang

Hi Geliang, Dan,

Quoted 1 line · Geliang Tang

Fix the following Smatch complaint:

Thanks for the report and the patch!

Quoted 17 lines

net/mptcp/pm_netlink.c:213 mptcp_pm_add_timer()
warn: variable dereferenced before check 'msk' (see line 208)

  net/mptcp/pm_netlink.c
     207          struct mptcp_sock *msk = entry->sock;
     208          struct sock *sk = (struct sock *)msk;
     209          struct net *net = sock_net(sk);
                                            ^^
  "msk" dereferenced here.

     210
     211          pr_debug("msk=%p", msk);
     212
     213          if (!msk)
                     ^^^^

Too late.

Quoted 7 lines

     214                  return;
     215
Fixes 93f323b9cccc ("mptcp: add a new sysctl add_addr_timeout")
Reported-by Dan Carpenter <dan.carpenter(a)oracle.com>
Signed-off-by Geliang Tang <geliangtang(a)gmail.com>
Reviewed-by Dan Carpenter <dan.carpenter(a)oracle.com>

A small detail (I think): the Signed-off-by of the sender (Geliang) should be the last one in the list if I am not mistaken. But I guess this is not blocking.

Reviewed-by Matthieu Baerts <matthieu.baerts(a)tessares.net>

Cheers,
Matt

Signature · 2 lines

Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

Dan Carpenter

2020-11-09 18:34 UTC #5 lore raw

Re: [MPTCP][PATCH net 1/2] mptcp: fix static checker warnings in mptcp_pm_add_timer

Quoted 27 lines · Matthieu Baerts

Hi Geliang, Dan,
Quoted 1 line · Geliang Tang
Fix the following Smatch complaint:
Thanks for the report and the patch!
Quoted 20 lines
net/mptcp/pm_netlink.c:213 mptcp_pm_add_timer()
warn: variable dereferenced before check 'msk' (see line 208)
  net/mptcp/pm_netlink.c
     207          struct mptcp_sock *msk = entry->sock;
     208          struct sock *sk = (struct sock *)msk;
     209          struct net *net = sock_net(sk);
                                            ^^
  "msk" dereferenced here.
     210
     211          pr_debug("msk=%p", msk);
     212
     213          if (!msk)
                     ^^^^
Too late.
     214                  return;
     215

Quoted 8 lines

Quoted 4 lines
Fixes 93f323b9cccc ("mptcp: add a new sysctl add_addr_timeout")
Reported-by Dan Carpenter <dan.carpenter(a)oracle.com>
Signed-off-by Geliang Tang <geliangtang(a)gmail.com>
Reviewed-by Dan Carpenter <dan.carpenter(a)oracle.com>
A small detail (I think): the Signed-off-by of the sender (Geliang) should be the last one in the list if I am not mistaken.
But I guess this is not blocking.

Generally, I like them to be in chronological order. For other tags like here it doesn't matter, but for signed-off-bys they only make sense in chronological order.

regards,
dan carpenter

Jakub Kicinski

maintainer

2020-11-09 20:56 UTC #6 lore raw

Re: [MPTCP][PATCH net 1/2] mptcp: fix static checker warnings in mptcp_pm_add_timer

Quoted 1 line · On Mon, 9 Nov 2020 21:34:19 +0300 Dan Carpenter wrote:

Generally, I like them to be in chronological order.

Jakub Kicinski

maintainer

2020-11-09 20:57 UTC #7 lore raw

Re: [MPTCP][PATCH net 1/2] mptcp: fix static checker warnings in mptcp_pm_add_timer

In reply to Matthieu Baerts

Quoted 5 lines · On Mon, 9 Nov 2020 17:28:54 +0100 Matthieu Baerts wrote:

A small detail (I think): the Signed-off-by of the sender (Geliang) should be the last one in the list if I am not mistaken. But I guess this is not blocking.
Reviewed-by Matthieu Baerts <matthieu.baerts(a)tessares.net>

I take it you'd like me to apply patch 1 directly to net?

Or do you prefer to take it into mptcp tree first?

Matthieu Baerts

2020-11-09 21:23 UTC #8 lore raw

Re: [MPTCP][PATCH net 1/2] mptcp: fix static checker warnings in mptcp_pm_add_timer

Hi Jakub,

09 Nov 2020 21:57:05 Jakub Kicinski <kuba(a)kernel.org>:

Quoted 8 lines · On Mon, 9 Nov 2020 17:28:54 +0100 Matthieu Baerts wrote:

Quoted 5 lines
A small detail (I think): the Signed-off-by of the sender (Geliang) should be the last one in the list if I am not mistaken. But I guess this is not blocking.
Reviewed-by Matthieu Baerts <matthieu.baerts(a)tessares.net>
I take it you'd like me to apply patch 1 directly to net?

Sorry, I didn't know it was OK to apply only one patch of the series. Then yes, if you don't mind, please apply this patch :)

Cheers,
Matt
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

Jakub Kicinski

maintainer

2020-11-09 22:20 UTC #9 lore raw

Re: [MPTCP][PATCH net 1/2] mptcp: fix static checker warnings in mptcp_pm_add_timer

Quoted 6 lines · On Mon, 9 Nov 2020 21:23:33 +0000 (UTC) Matthieu Baerts wrote:

09 Nov 2020 21:57:05 Jakub Kicinski <kuba(a)kernel.org>:
Quoted 5 lines · On Mon, 9 Nov 2020 17:28:54 +0100 Matthieu Baerts wrote:
Quoted 4 lines
A small detail (I think): the Signed-off-by of the sender (Geliang) should be the last one in the list if I am not mistaken. But I guess this is not blocking.

Quoted 6 lines

Quoted 3 lines
Quoted 1 line
Reviewed-by Matthieu Baerts <matthieu.baerts(a)tessares.net>
I take it you'd like me to apply patch 1 directly to net?
Sorry, I didn't know it was OK to apply only one patch of the series. Then yes, if you don't mind, please apply this patch :)

Not really, I was just establishing ownership ;)

Geliang Tang, please rebase on net and repost just the first patch. It does not apply to net as is.

Geliang Tang

author

2020-11-12 02:36 UTC #10 lore raw

Re: [MPTCP][PATCH net 1/2] mptcp: fix static checker warnings in mptcp_pm_add_timer

Hi Jakub, Matt,

Quoted 12 lines · Jakub Kicinski <kuba(a)kernel.org> 于2020年11月10日周二上午6:20写道：

Quoted 10 lines · On Mon, 9 Nov 2020 21:23:33 +0000 (UTC) Matthieu Baerts wrote:
09 Nov 2020 21:57:05 Jakub Kicinski <kuba(a)kernel.org>:
Quoted 8 lines · On Mon, 9 Nov 2020 17:28:54 +0100 Matthieu Baerts wrote:
Quoted 5 lines
A small detail (I think): the Signed-off-by of the sender (Geliang) should be the last one in the list if I am not mistaken. But I guess this is not blocking.
Reviewed-by Matthieu Baerts <matthieu.baerts(a)tessares.net>
I take it you'd like me to apply patch 1 directly to net?

Quoted 7 lines

Quoted 2 lines
Sorry, I didn't know it was OK to apply only one patch of the series. Then yes, if you don't mind, please apply this patch :)
Not really, I was just establishing ownership ;)
Geliang Tang, please rebase on net and repost just the first patch. It does not apply to net as is.

v2 of this patch had been sent out.

http://patchwork.ozlabs.org/project/netdev/patch/078a2ef5bdc4e3b2c25ef852461692001f426495.1604976945.git.geliangtang(a)gmail.com/

This patch should be applied to net-next, not -net. Since commit "mptcp:
add a new sysctl add_addr_timeout" is not applied to -net yet.

-Geliang