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 <[email protected]>
Signed-off-by: Geliang Tang <[email protected]>
Reviewed-by: Dan Carpenter <[email protected]>
---
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
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 <[email protected]>
Reviewed-by: Dan Carpenter <[email protected]>
---
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
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 <[email protected]>
> Signed-off-by: Geliang Tang <[email protected]>
> Reviewed-by: Dan Carpenter <[email protected]>
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 <[email protected]>
Cheers,
Matt
--
Tessares | Belgium | Hybrid Access Solutions
http://www.tessares.net
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 <[email protected]>
> Reviewed-by: Dan Carpenter <[email protected]>
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
http://www.tessares.net
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 <[email protected]>
> > Signed-off-by: Geliang Tang <[email protected]>
> > Reviewed-by: Dan Carpenter <[email protected]>
>
> 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
On Mon, 9 Nov 2020 21:34:19 +0300 Dan Carpenter wrote:
> Generally, I like them to be in chronological order.
+1
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 <[email protected]>
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?
Hi Jakub,
09 Nov 2020 21:57:05 Jakub Kicinski <[email protected]>:
> 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 <[email protected]>
>
> 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
http://www.tessares.net
On Mon, 9 Nov 2020 21:23:33 +0000 (UTC) Matthieu Baerts wrote:
> 09 Nov 2020 21:57:05 Jakub Kicinski <[email protected]>:
> > 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 <[email protected]>
> >
> > 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.
Hi Jakub, Matt,
Jakub Kicinski <[email protected]> 于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 <[email protected]>:
> > > 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 <[email protected]>
> > >
> > > 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