Re: ipmr, ip6mr: Unite dumproute flows

Re: ipmr, ip6mr: Unite dumproute flows

[Colin Ian King]

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

Re: ipmr, ip6mr: Unite dumproute flows

[Nikolay Aleksandrov]

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

[PATCH net] net: ipmr: fix unresolved entry dumps

[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

Re: [PATCH net] net: ipmr: fix unresolved entry dumps

[Nikolay Aleksandrov]

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.

Re: [PATCH net] net: ipmr: fix unresolved entry dumps

[David Miller]

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.

Re: [PATCH net] net: ipmr: fix unresolved entry dumps

[David Ahern]

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.

Re: [PATCH net] net: ipmr: fix unresolved entry dumps

[David Miller]

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.