diff mbox series

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

Message ID ccf004469e02fb5bd7ec822414b9a98b0015f4a3.1604930005.git.geliangtang@gmail.com (mailing list archive)
State Not Applicable
Delegated to: Netdev Maintainers
Headers show
Series fix static checker warnings in | expand

Checks

Context Check Description
netdev/apply fail Patch does not apply to net
netdev/tree_selection success Clearly marked for net

Commit Message

Geliang Tang Nov. 9, 2020, 1:59 p.m. UTC
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(-)

Comments

Matthieu Baerts Nov. 9, 2020, 4:28 p.m. UTC | #1
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
Dan Carpenter Nov. 9, 2020, 6:34 p.m. UTC | #2
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
Jakub Kicinski Nov. 9, 2020, 8:56 p.m. UTC | #3
On Mon, 9 Nov 2020 21:34:19 +0300 Dan Carpenter wrote:
> Generally, I like them to be in chronological order. 

+1
Jakub Kicinski Nov. 9, 2020, 8:57 p.m. UTC | #4
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?
Matthieu Baerts Nov. 9, 2020, 9:23 p.m. UTC | #5
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
Jakub Kicinski Nov. 9, 2020, 10:20 p.m. UTC | #6
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.
Geliang Tang Nov. 12, 2020, 2:36 a.m. UTC | #7
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
diff mbox series

Patch

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