* Re: ipmr, ip6mr: Unite dumproute flows
@ 2018-10-17 18:22 Colin Ian King
2018-10-17 18:49 ` Nikolay Aleksandrov
0 siblings, 1 reply; 7+ messages in thread
From: Colin Ian King @ 2018-10-17 18:22 UTC (permalink / raw)
To: Yuval Mintz, Nikolay Aleksandrov
Cc: David S. Miller, Alexey Kuznetsov, Hideaki YOSHIFUJI, netdev
Hi,
Static analysis on linux-next has picked up a potential bug with commit:
commit 7b0db85737db3f4d76b2a412e4f19eae59b8b494
Author: Yuval Mintz <yuvalm@mellanox.com>
Date: Wed Feb 28 23:29:39 2018 +0200
ipmr, ip6mr: Unite dumproute flows
in function mr_table_dump(), s_e is and never seems to change, so the
check if (e < s_e) is never true: See code below, as annotated by
CoverityScan:
317 }
assignment: Assigning: e = 0U.
318 e = 0;
assignment: Assigning: s_e = 0U.
319 s_e = 0;
320
321 spin_lock_bh(lock);
322 list_for_each_entry(mfc, &mrt->mfc_unres_queue, list) {
at_least: At condition e < s_e, the value of e must be at least 0.
const: At condition e < s_e, the value of s_e must be equal to 0.
dead_error_condition: The condition e < s_e cannot be true.
323 if (e < s_e)
CID 1474511 (#1 of 1): Logically dead code (DEADCODE)
dead_error_line: Execution cannot reach this statement: goto next_entry2;.
324 goto next_entry2;
325 if (filter->dev &&
326 !mr_mfc_uses_dev(mrt, mfc, filter->dev))
327 goto next_entry2;
328
329 err = fill(mrt, skb, NETLINK_CB(cb->skb).portid,
330 cb->nlh->nlmsg_seq, mfc, RTM_NEWROUTE, flags);
331 if (err < 0) {
332 spin_unlock_bh(lock);
333 goto out;
334 }
335next_entry2:
incr: Incrementing e. The value of e is now 1.
incr: Incrementing e. The value of e is now 2.
336 e++;
Note that the zero'ing of e and s_e for ipmr and ip6mr was added in by
earlier commits:
commit 8fb472c09b9df478a062eacc7841448e40fc3c17
Author: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
Date: Thu Jan 12 15:53:33 2017 +0100
ipmr: improve hash scalability
commit 87c418bf1323d57140f4b448715f64de3fbb7e91
Author: Yuval Mintz <yuvalm@mellanox.com>
Date: Wed Feb 28 23:29:31 2018 +0200
ip6mr: Align hash implementation to ipmr
Anyhow, this looks incorrect, but I'm not familiar with this code to
suggest the correct fix.
Colin
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: ipmr, ip6mr: Unite dumproute flows
2018-10-17 18:22 ipmr, ip6mr: Unite dumproute flows Colin Ian King
@ 2018-10-17 18:49 ` Nikolay Aleksandrov
2018-10-17 19:34 ` [PATCH net] net: ipmr: fix unresolved entry dumps Nikolay Aleksandrov
0 siblings, 1 reply; 7+ messages in thread
From: Nikolay Aleksandrov @ 2018-10-17 18:49 UTC (permalink / raw)
To: Colin Ian King, Yuval Mintz
Cc: David S. Miller, Alexey Kuznetsov, Hideaki YOSHIFUJI, netdev
On 17/10/2018 21:22, Colin Ian King wrote:
> Hi,
>
> Static analysis on linux-next has picked up a potential bug with commit:
>
> commit 7b0db85737db3f4d76b2a412e4f19eae59b8b494
> Author: Yuval Mintz <yuvalm@mellanox.com>
> Date: Wed Feb 28 23:29:39 2018 +0200
>
> ipmr, ip6mr: Unite dumproute flows
>
> in function mr_table_dump(), s_e is and never seems to change, so the
> check if (e < s_e) is never true: See code below, as annotated by
> CoverityScan:
>
> 317 }
> assignment: Assigning: e = 0U.
>
> 318 e = 0;
> assignment: Assigning: s_e = 0U.
>
> 319 s_e = 0;
> 320
> 321 spin_lock_bh(lock);
> 322 list_for_each_entry(mfc, &mrt->mfc_unres_queue, list) {
> at_least: At condition e < s_e, the value of e must be at least 0.
> const: At condition e < s_e, the value of s_e must be equal to 0.
> dead_error_condition: The condition e < s_e cannot be true.
>
> 323 if (e < s_e)
>
> CID 1474511 (#1 of 1): Logically dead code (DEADCODE)
> dead_error_line: Execution cannot reach this statement: goto next_entry2;.
>
> 324 goto next_entry2;
> 325 if (filter->dev &&
> 326 !mr_mfc_uses_dev(mrt, mfc, filter->dev))
> 327 goto next_entry2;
> 328
> 329 err = fill(mrt, skb, NETLINK_CB(cb->skb).portid,
> 330 cb->nlh->nlmsg_seq, mfc, RTM_NEWROUTE, flags);
> 331 if (err < 0) {
> 332 spin_unlock_bh(lock);
> 333 goto out;
> 334 }
> 335next_entry2:
>
> incr: Incrementing e. The value of e is now 1.
> incr: Incrementing e. The value of e is now 2.
>
> 336 e++;
>
>
> Note that the zero'ing of e and s_e for ipmr and ip6mr was added in by
> earlier commits:
>
> commit 8fb472c09b9df478a062eacc7841448e40fc3c17
> Author: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
> Date: Thu Jan 12 15:53:33 2017 +0100
>
> ipmr: improve hash scalability
>
> commit 87c418bf1323d57140f4b448715f64de3fbb7e91
> Author: Yuval Mintz <yuvalm@mellanox.com>
> Date: Wed Feb 28 23:29:31 2018 +0200
>
> ip6mr: Align hash implementation to ipmr
>
> Anyhow, this looks incorrect, but I'm not familiar with this code to
> suggest the correct fix.
>
> Colin
>
Indeed, there's a case where we might miss an unresolved entry due to the zeroing.
I'll send a fix shortly after testing.
Thanks,
Nik
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH net] net: ipmr: fix unresolved entry dumps
2018-10-17 18:49 ` Nikolay Aleksandrov
@ 2018-10-17 19:34 ` Nikolay Aleksandrov
2018-10-17 19:35 ` Nikolay Aleksandrov
2018-10-18 5:36 ` David Miller
0 siblings, 2 replies; 7+ messages in thread
From: Nikolay Aleksandrov @ 2018-10-17 19:34 UTC (permalink / raw)
To: netdev; +Cc: davem, Nikolay Aleksandrov
If the skb space ends in an unresolved entry while dumping we'll miss
some unresolved entries. The reason is due to zeroing the entry counter
between dumping resolved and unresolved mfc entries. We should just
keep counting until the whole table is dumped and zero when we move to
the next as we have a separate table counter.
Reported-by: Colin Ian King <colin.king@canonical.com>
Fixes: 8fb472c09b9d ("ipmr: improve hash scalability")
Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
---
Dropped Yuval's mail because it bounces.
net/ipv4/ipmr_base.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/net/ipv4/ipmr_base.c b/net/ipv4/ipmr_base.c
index 1ad9aa62a97b..eab8cd5ec2f5 100644
--- a/net/ipv4/ipmr_base.c
+++ b/net/ipv4/ipmr_base.c
@@ -296,8 +296,6 @@ int mr_rtm_dumproute(struct sk_buff *skb, struct netlink_callback *cb,
next_entry:
e++;
}
- e = 0;
- s_e = 0;
spin_lock_bh(lock);
list_for_each_entry(mfc, &mrt->mfc_unres_queue, list) {
--
2.17.2
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH net] net: ipmr: fix unresolved entry dumps
2018-10-17 19:34 ` [PATCH net] net: ipmr: fix unresolved entry dumps Nikolay Aleksandrov
@ 2018-10-17 19:35 ` Nikolay Aleksandrov
2018-10-18 5:36 ` David Miller
1 sibling, 0 replies; 7+ messages in thread
From: Nikolay Aleksandrov @ 2018-10-17 19:35 UTC (permalink / raw)
To: netdev; +Cc: davem, Colin Ian King
On 17/10/2018 22:34, Nikolay Aleksandrov wrote:
> If the skb space ends in an unresolved entry while dumping we'll miss
> some unresolved entries. The reason is due to zeroing the entry counter
> between dumping resolved and unresolved mfc entries. We should just
> keep counting until the whole table is dumped and zero when we move to
> the next as we have a separate table counter.
>
> Reported-by: Colin Ian King <colin.king@canonical.com>
> Fixes: 8fb472c09b9d ("ipmr: improve hash scalability")
> Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
> ---
> Dropped Yuval's mail because it bounces.
>
> net/ipv4/ipmr_base.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/net/ipv4/ipmr_base.c b/net/ipv4/ipmr_base.c
> index 1ad9aa62a97b..eab8cd5ec2f5 100644
> --- a/net/ipv4/ipmr_base.c
> +++ b/net/ipv4/ipmr_base.c
> @@ -296,8 +296,6 @@ int mr_rtm_dumproute(struct sk_buff *skb, struct netlink_callback *cb,
> next_entry:
> e++;
> }
> - e = 0;
> - s_e = 0;
>
> spin_lock_bh(lock);
> list_for_each_entry(mfc, &mrt->mfc_unres_queue, list) {
>
+CC Colin
Sorry about that, my script somehow missed the reported-by email.
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH net] net: ipmr: fix unresolved entry dumps
2018-10-17 19:34 ` [PATCH net] net: ipmr: fix unresolved entry dumps Nikolay Aleksandrov
2018-10-17 19:35 ` Nikolay Aleksandrov
@ 2018-10-18 5:36 ` David Miller
2018-10-18 15:16 ` David Ahern
1 sibling, 1 reply; 7+ messages in thread
From: David Miller @ 2018-10-18 5:36 UTC (permalink / raw)
To: nikolay; +Cc: netdev
From: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
Date: Wed, 17 Oct 2018 22:34:34 +0300
> If the skb space ends in an unresolved entry while dumping we'll miss
> some unresolved entries. The reason is due to zeroing the entry counter
> between dumping resolved and unresolved mfc entries. We should just
> keep counting until the whole table is dumped and zero when we move to
> the next as we have a separate table counter.
>
> Reported-by: Colin Ian King <colin.king@canonical.com>
> Fixes: 8fb472c09b9d ("ipmr: improve hash scalability")
> Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
Applied and queued up for -stable.
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH net] net: ipmr: fix unresolved entry dumps
2018-10-18 5:36 ` David Miller
@ 2018-10-18 15:16 ` David Ahern
2018-10-18 23:46 ` David Miller
0 siblings, 1 reply; 7+ messages in thread
From: David Ahern @ 2018-10-18 15:16 UTC (permalink / raw)
To: David Miller, nikolay; +Cc: netdev
On 10/17/18 11:36 PM, David Miller wrote:
> From: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
> Date: Wed, 17 Oct 2018 22:34:34 +0300
>
>> If the skb space ends in an unresolved entry while dumping we'll miss
>> some unresolved entries. The reason is due to zeroing the entry counter
>> between dumping resolved and unresolved mfc entries. We should just
>> keep counting until the whole table is dumped and zero when we move to
>> the next as we have a separate table counter.
>>
>> Reported-by: Colin Ian King <colin.king@canonical.com>
>> Fixes: 8fb472c09b9d ("ipmr: improve hash scalability")
>> Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
>
> Applied and queued up for -stable.
>
FYI: on the net to net-next merge, those 2 lines were moved to
mr_table_dump.
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH net] net: ipmr: fix unresolved entry dumps
2018-10-18 15:16 ` David Ahern
@ 2018-10-18 23:46 ` David Miller
0 siblings, 0 replies; 7+ messages in thread
From: David Miller @ 2018-10-18 23:46 UTC (permalink / raw)
To: dsahern; +Cc: nikolay, netdev
From: David Ahern <dsahern@gmail.com>
Date: Thu, 18 Oct 2018 09:16:17 -0600
> On 10/17/18 11:36 PM, David Miller wrote:
>> From: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
>> Date: Wed, 17 Oct 2018 22:34:34 +0300
>>
>>> If the skb space ends in an unresolved entry while dumping we'll miss
>>> some unresolved entries. The reason is due to zeroing the entry counter
>>> between dumping resolved and unresolved mfc entries. We should just
>>> keep counting until the whole table is dumped and zero when we move to
>>> the next as we have a separate table counter.
>>>
>>> Reported-by: Colin Ian King <colin.king@canonical.com>
>>> Fixes: 8fb472c09b9d ("ipmr: improve hash scalability")
>>> Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
>>
>> Applied and queued up for -stable.
>>
>
> FYI: on the net to net-next merge, those 2 lines were moved to
> mr_table_dump.
Thanks for the heads up.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-10-19 7:49 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-17 18:22 ipmr, ip6mr: Unite dumproute flows Colin Ian King
2018-10-17 18:49 ` Nikolay Aleksandrov
2018-10-17 19:34 ` [PATCH net] net: ipmr: fix unresolved entry dumps Nikolay Aleksandrov
2018-10-17 19:35 ` Nikolay Aleksandrov
2018-10-18 5:36 ` David Miller
2018-10-18 15:16 ` David Ahern
2018-10-18 23:46 ` David Miller
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.