[MPTCP][PATCH net 0/2] fix static checker warnings in

[MPTCP][PATCH net 0/2] fix static checker warnings in

[Geliang Tang]

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(-)

-- 
2.26.2

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

[Geliang Tang]

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@oracle.com>
Signed-off-by: Geliang Tang <geliangtang@gmail.com>
Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 net/mptcp/pm_netlink.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index 6180a8b39a3f..03f2c28f11f5 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -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);
 
-- 
2.26.2

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

[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@gmail.com>
Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 net/mptcp/pm_netlink.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index 03f2c28f11f5..dfc1bed4a55f 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -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;
 }
-- 
2.26.2

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

[Matthieu Baerts]

Hi Geliang, Dan,

On 09/11/2020 14:59, Geliang Tang wrote:
> Fix the following Smatch complaint:

Thanks for the report and the patch!

>       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@oracle.com>
> Signed-off-by: Geliang Tang <geliangtang@gmail.com>
> Reviewed-by: Dan Carpenter <dan.carpenter@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@tessares.net>

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

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

[Matthieu Baerts]

Hi Geliang,

On 09/11/2020 14:59, Geliang Tang wrote:
> 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@gmail.com>
> Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>

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

> ---
>   net/mptcp/pm_netlink.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index 03f2c28f11f5..dfc1bed4a55f 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -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
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

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

[Dan Carpenter]

On Mon, Nov 09, 2020 at 05:28:54PM +0100, Matthieu Baerts wrote:
> Hi Geliang, Dan,
> 
> On 09/11/2020 14:59, Geliang Tang wrote:
> > Fix the following Smatch complaint:
> 
> Thanks for the report and the patch!
> 
> >       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@oracle.com>
> > Signed-off-by: Geliang Tang <geliangtang@gmail.com>
> > Reviewed-by: Dan Carpenter <dan.carpenter@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

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

[Jakub Kicinski]

On Mon, 9 Nov 2020 21:34:19 +0300 Dan Carpenter wrote:
> Generally, I like them to be in chronological order. 

+1

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

[Jakub Kicinski]

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@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?

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

[Matthieu Baerts]

Hi Jakub,

09 Nov 2020 21:57:05 Jakub Kicinski <kuba@kernel.org>:

> 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@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

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

[Jakub Kicinski]

On Mon, 9 Nov 2020 21:23:33 +0000 (UTC) Matthieu Baerts wrote:
> 09 Nov 2020 21:57:05 Jakub Kicinski <kuba@kernel.org>:
> > 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@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.

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

[Geliang Tang]

Hi Jakub, Matt,

Jakub Kicinski <kuba@kernel.org> 于2020年11月10日周二 上午6:20写道：
>
> On Mon, 9 Nov 2020 21:23:33 +0000 (UTC) Matthieu Baerts wrote:
> > 09 Nov 2020 21:57:05 Jakub Kicinski <kuba@kernel.org>:
> > > 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@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.

v2 of this patch had been sent out.

http://patchwork.ozlabs.org/project/netdev/patch/078a2ef5bdc4e3b2c25ef852461692001f426495.1604976945.git.geliangtang@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