| Message ID | 1480296725-5563-1-git-send-email-zhangshengju@cmss.chinamobile.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On 11/27/16 6:32 PM, Zhang Shengju wrote: > Loop index in neigh dump function is not updated correctly under some > circumstances, this patch will fix it. What's an example? > > Fixes: 16660f0bd9 ("net: Add support for filtering neigh dump by device index") > Fixes: 21fdd092ac ("net: Add support for filtering neigh dump by master device") > > Signed-off-by: Zhang Shengju <zhangshengju@cmss.chinamobile.com> > --- > net/core/neighbour.c | 39 ++++++++++++++++++--------------------- > 1 file changed, 18 insertions(+), 21 deletions(-) > > diff --git a/net/core/neighbour.c b/net/core/neighbour.c > index 2ae929f..ce32e9c 100644 > --- a/net/core/neighbour.c > +++ b/net/core/neighbour.c > @@ -2256,6 +2256,16 @@ static bool neigh_ifindex_filtered(struct net_device *dev, int filter_idx) > return false; > } > > +static bool neigh_dump_filtered(struct net_device *dev, int filter_idx, > + int filter_master_idx) > +{ > + if (neigh_ifindex_filtered(dev, filter_idx) || > + neigh_master_filtered(dev, filter_master_idx)) > + return true; > + > + return false; > +} > + > static int neigh_dump_table(struct neigh_table *tbl, struct sk_buff *skb, > struct netlink_callback *cb) > { > @@ -2285,20 +2295,15 @@ static int neigh_dump_table(struct neigh_table *tbl, struct sk_buff *skb, > rcu_read_lock_bh(); > nht = rcu_dereference_bh(tbl->nht); > > - for (h = s_h; h < (1 << nht->hash_shift); h++) { > - if (h > s_h) > - s_idx = 0; > + for (h = s_h; h < (1 << nht->hash_shift); h++, s_idx = 0) { > for (n = rcu_dereference_bh(nht->hash_buckets[h]), idx = 0; > n != NULL; > - n = rcu_dereference_bh(n->next)) { > - if (!net_eq(dev_net(n->dev), net)) > - continue; > - if (neigh_ifindex_filtered(n->dev, filter_idx)) > + n = rcu_dereference_bh(n->next), idx++) { > + if (idx < s_idx || !net_eq(dev_net(n->dev), net)) > continue; > - if (neigh_master_filtered(n->dev, filter_master_idx)) > + if (neigh_dump_filtered(n->dev, filter_idx, > + filter_master_idx)) > continue; > - if (idx < s_idx) > - goto next; > if (neigh_fill_info(skb, n, NETLINK_CB(cb->skb).portid, > cb->nlh->nlmsg_seq, > RTM_NEWNEIGH, > @@ -2306,8 +2311,6 @@ static int neigh_dump_table(struct neigh_table *tbl, struct sk_buff *skb, > rc = -1; > goto out; > } > -next: > - idx++; > } > } > rc = skb->len; > @@ -2328,14 +2331,10 @@ static int pneigh_dump_table(struct neigh_table *tbl, struct sk_buff *skb, > > read_lock_bh(&tbl->lock); > > - for (h = s_h; h <= PNEIGH_HASHMASK; h++) { > - if (h > s_h) > - s_idx = 0; > - for (n = tbl->phash_buckets[h], idx = 0; n; n = n->next) { > - if (pneigh_net(n) != net) > + for (h = s_h; h <= PNEIGH_HASHMASK; h++, s_idx = 0) { > + for (n = tbl->phash_buckets[h], idx = 0; n; n = n->next, idx++) { > + if (idx < s_idx || pneigh_net(n) != net) > continue; > - if (idx < s_idx) > - goto next; > if (pneigh_fill_info(skb, n, NETLINK_CB(cb->skb).portid, > cb->nlh->nlmsg_seq, > RTM_NEWNEIGH, > @@ -2344,8 +2343,6 @@ static int pneigh_dump_table(struct neigh_table *tbl, struct sk_buff *skb, > rc = -1; > goto out; > } > - next: > - idx++; > } > } > This fix is way to be complicated to be fixing anything related to 16660f0bd9 or 21fdd092ac. Both of those commits added a continue: if (neigh_ifindex_filtered(n->dev, filter_idx)) continue; if (neigh_master_filtered(n->dev, filter_master_idx)) continue; At best the continue is replaced by 'goto next;' and I am not convinced that is right. You are completely rewriting the dump loops.
> -----Original Message----- > From: David Ahern [mailto:dsa@cumulusnetworks.com] > Sent: Monday, November 28, 2016 10:10 AM > To: Zhang Shengju <zhangshengju@cmss.chinamobile.com>; > netdev@vger.kernel.org > Subject: Re: [net,v2] neigh: fix the loop index error in neigh dump > > On 11/27/16 6:32 PM, Zhang Shengju wrote: > > Loop index in neigh dump function is not updated correctly under some > > circumstances, this patch will fix it. > > What's an example? If dev is filtered out, the original code goes to next loop without updating loop index 'idx'. > > > > > Fixes: 16660f0bd9 ("net: Add support for filtering neigh dump by > > device index") > > Fixes: 21fdd092ac ("net: Add support for filtering neigh dump by > > master device") > > > > Signed-off-by: Zhang Shengju <zhangshengju@cmss.chinamobile.com> > > --- > > net/core/neighbour.c | 39 ++++++++++++++++++--------------------- > > 1 file changed, 18 insertions(+), 21 deletions(-) > > > > diff --git a/net/core/neighbour.c b/net/core/neighbour.c index > > 2ae929f..ce32e9c 100644 > > --- a/net/core/neighbour.c > > +++ b/net/core/neighbour.c > > @@ -2256,6 +2256,16 @@ static bool neigh_ifindex_filtered(struct > net_device *dev, int filter_idx) > > return false; > > } > > > > +static bool neigh_dump_filtered(struct net_device *dev, int filter_idx, > > + int filter_master_idx) > > +{ > > + if (neigh_ifindex_filtered(dev, filter_idx) || > > + neigh_master_filtered(dev, filter_master_idx)) > > + return true; > > + > > + return false; > > +} > > + > > static int neigh_dump_table(struct neigh_table *tbl, struct sk_buff *skb, > > struct netlink_callback *cb) > > { > > @@ -2285,20 +2295,15 @@ static int neigh_dump_table(struct neigh_table > *tbl, struct sk_buff *skb, > > rcu_read_lock_bh(); > > nht = rcu_dereference_bh(tbl->nht); > > > > - for (h = s_h; h < (1 << nht->hash_shift); h++) { > > - if (h > s_h) > > - s_idx = 0; > > + for (h = s_h; h < (1 << nht->hash_shift); h++, s_idx = 0) { > > for (n = rcu_dereference_bh(nht->hash_buckets[h]), idx = 0; > > n != NULL; > > - n = rcu_dereference_bh(n->next)) { > > - if (!net_eq(dev_net(n->dev), net)) > > - continue; > > - if (neigh_ifindex_filtered(n->dev, filter_idx)) > > + n = rcu_dereference_bh(n->next), idx++) { > > + if (idx < s_idx || !net_eq(dev_net(n->dev), net)) > > continue; > > - if (neigh_master_filtered(n->dev, filter_master_idx)) > > + if (neigh_dump_filtered(n->dev, filter_idx, > > + filter_master_idx)) > > continue; > > - if (idx < s_idx) > > - goto next; > > if (neigh_fill_info(skb, n, NETLINK_CB(cb- > >skb).portid, > > cb->nlh->nlmsg_seq, > > RTM_NEWNEIGH, > > @@ -2306,8 +2311,6 @@ static int neigh_dump_table(struct neigh_table > *tbl, struct sk_buff *skb, > > rc = -1; > > goto out; > > } > > -next: > > - idx++; > > } > > } > > rc = skb->len; > > @@ -2328,14 +2331,10 @@ static int pneigh_dump_table(struct > > neigh_table *tbl, struct sk_buff *skb, > > > > read_lock_bh(&tbl->lock); > > > > - for (h = s_h; h <= PNEIGH_HASHMASK; h++) { > > - if (h > s_h) > > - s_idx = 0; > > - for (n = tbl->phash_buckets[h], idx = 0; n; n = n->next) { > > - if (pneigh_net(n) != net) > > + for (h = s_h; h <= PNEIGH_HASHMASK; h++, s_idx = 0) { > > + for (n = tbl->phash_buckets[h], idx = 0; n; n = n->next, idx++) > { > > + if (idx < s_idx || pneigh_net(n) != net) > > continue; > > - if (idx < s_idx) > > - goto next; > > if (pneigh_fill_info(skb, n, NETLINK_CB(cb- > >skb).portid, > > cb->nlh->nlmsg_seq, > > RTM_NEWNEIGH, > > @@ -2344,8 +2343,6 @@ static int pneigh_dump_table(struct neigh_table > *tbl, struct sk_buff *skb, > > rc = -1; > > goto out; > > } > > - next: > > - idx++; > > } > > } > > > > This fix is way to be complicated to be fixing anything related to 16660f0bd9 > or 21fdd092ac. Both of those commits added a continue: > > if (neigh_ifindex_filtered(n->dev, filter_idx)) > continue; > if (neigh_master_filtered(n->dev, filter_master_idx)) > continue; > > At best the continue is replaced by 'goto next;' and I am not convinced that is > right. > > You are completely rewriting the dump loops. I put 'idx++' into for loop, so I replace 'goto' with 'continue'. The other change is style related. Thanks, Zhang Shengju
On 11/27/16 7:34 PM, 张胜举 wrote: >> -----Original Message----- >> From: David Ahern [mailto:dsa@cumulusnetworks.com] >> Sent: Monday, November 28, 2016 10:10 AM >> To: Zhang Shengju <zhangshengju@cmss.chinamobile.com>; >> netdev@vger.kernel.org >> Subject: Re: [net,v2] neigh: fix the loop index error in neigh dump >> >> On 11/27/16 6:32 PM, Zhang Shengju wrote: >>> Loop index in neigh dump function is not updated correctly under some >>> circumstances, this patch will fix it. >> >> What's an example? > > If dev is filtered out, the original code goes to next loop without updating > loop index 'idx'. And you have a use case with missing or redundant data? Or is your comment based on a review of code only? >> You are completely rewriting the dump loops. > > I put 'idx++' into for loop, so I replace 'goto' with 'continue'. The > other change is style related. A "fixes" should not include 'style related' changes.
> -----Original Message----- > From: David Ahern [mailto:dsa@cumulusnetworks.com] > Sent: Monday, November 28, 2016 10:39 AM > To: 张胜举 <zhangshengju@cmss.chinamobile.com>; > netdev@vger.kernel.org > Subject: Re: [net,v2] neigh: fix the loop index error in neigh dump > > On 11/27/16 7:34 PM, 张胜举 wrote: > >> -----Original Message----- > >> From: David Ahern [mailto:dsa@cumulusnetworks.com] > >> Sent: Monday, November 28, 2016 10:10 AM > >> To: Zhang Shengju <zhangshengju@cmss.chinamobile.com>; > >> netdev@vger.kernel.org > >> Subject: Re: [net,v2] neigh: fix the loop index error in neigh dump > >> > >> On 11/27/16 6:32 PM, Zhang Shengju wrote: > >>> Loop index in neigh dump function is not updated correctly under > >>> some circumstances, this patch will fix it. > >> > >> What's an example? > > > > If dev is filtered out, the original code goes to next loop without > > updating loop index 'idx'. > > And you have a use case with missing or redundant data? Or is your > comment based on a review of code only? It's on my code review. No use case currently, this is uncommon to happen. > > >> You are completely rewriting the dump loops. > > > > I put 'idx++' into for loop, so I replace 'goto' with 'continue'. > > The other change is style related. > > A "fixes" should not include 'style related' changes. Okay, I will send another version without style changes.
On 11/27/16 7:53 PM, 张胜举 wrote: > > >> -----Original Message----- >> From: David Ahern [mailto:dsa@cumulusnetworks.com] >> Sent: Monday, November 28, 2016 10:39 AM >> To: 张胜举 <zhangshengju@cmss.chinamobile.com>; >> netdev@vger.kernel.org >> Subject: Re: [net,v2] neigh: fix the loop index error in neigh dump >> >> On 11/27/16 7:34 PM, 张胜举 wrote: >>>> -----Original Message----- >>>> From: David Ahern [mailto:dsa@cumulusnetworks.com] >>>> Sent: Monday, November 28, 2016 10:10 AM >>>> To: Zhang Shengju <zhangshengju@cmss.chinamobile.com>; >>>> netdev@vger.kernel.org >>>> Subject: Re: [net,v2] neigh: fix the loop index error in neigh dump >>>> >>>> On 11/27/16 6:32 PM, Zhang Shengju wrote: >>>>> Loop index in neigh dump function is not updated correctly under >>>>> some circumstances, this patch will fix it. >>>> >>>> What's an example? >>> >>> If dev is filtered out, the original code goes to next loop without >>> updating loop index 'idx'. >> >> And you have a use case with missing or redundant data? Or is your >> comment based on a review of code only? > It's on my code review. No use case currently, this is uncommon to happen. > > >> >>>> You are completely rewriting the dump loops. >>> >>> I put 'idx++' into for loop, so I replace 'goto' with 'continue'. >>> The other change is style related. >> >> A "fixes" should not include 'style related' changes. > Okay, I will send another version without style changes. > Personally, I think you need to produce a use case that fails before sending another patch. I have not seen a problem with this code.
On 11/27/16 7:56 PM, David Ahern wrote: > On 11/27/16 7:53 PM, 张胜举 wrote: >> >> >>> -----Original Message----- >>> From: David Ahern [mailto:dsa@cumulusnetworks.com] >>> Sent: Monday, November 28, 2016 10:39 AM >>> To: 张胜举 <zhangshengju@cmss.chinamobile.com>; >>> netdev@vger.kernel.org >>> Subject: Re: [net,v2] neigh: fix the loop index error in neigh dump >>> >>> On 11/27/16 7:34 PM, 张胜举 wrote: >>>>> -----Original Message----- >>>>> From: David Ahern [mailto:dsa@cumulusnetworks.com] >>>>> Sent: Monday, November 28, 2016 10:10 AM >>>>> To: Zhang Shengju <zhangshengju@cmss.chinamobile.com>; >>>>> netdev@vger.kernel.org >>>>> Subject: Re: [net,v2] neigh: fix the loop index error in neigh dump >>>>> >>>>> On 11/27/16 6:32 PM, Zhang Shengju wrote: >>>>>> Loop index in neigh dump function is not updated correctly under >>>>>> some circumstances, this patch will fix it. >>>>> >>>>> What's an example? >>>> >>>> If dev is filtered out, the original code goes to next loop without >>>> updating loop index 'idx'. >>> >>> And you have a use case with missing or redundant data? Or is your >>> comment based on a review of code only? >> It's on my code review. No use case currently, this is uncommon to happen. >> >> >>> >>>>> You are completely rewriting the dump loops. >>>> >>>> I put 'idx++' into for loop, so I replace 'goto' with 'continue'. >>>> The other change is style related. >>> >>> A "fixes" should not include 'style related' changes. >> Okay, I will send another version without style changes. >> > > Personally, I think you need to produce a use case that fails before sending another patch. I have not seen a problem with this code. > And looking back at 3f0ae05d6f I should not have acked it (reviewed it too quickly while on PTO). Your change is a no-op because of what idx represents - the position in the hash list for devices relevant for the dump request. Same goes for the neigh dump so this patch is not needed.
> -----Original Message----- > From: David Ahern [mailto:dsa@cumulusnetworks.com] > Sent: Monday, November 28, 2016 11:10 AM > To: 张胜举 <zhangshengju@cmss.chinamobile.com>; > netdev@vger.kernel.org > Subject: Re: [net,v2] neigh: fix the loop index error in neigh dump > > On 11/27/16 7:56 PM, David Ahern wrote: > > On 11/27/16 7:53 PM, 张胜举 wrote: > >> > >> > >>> -----Original Message----- > >>> From: David Ahern [mailto:dsa@cumulusnetworks.com] > >>> Sent: Monday, November 28, 2016 10:39 AM > >>> To: 张胜举 <zhangshengju@cmss.chinamobile.com>; > >>> netdev@vger.kernel.org > >>> Subject: Re: [net,v2] neigh: fix the loop index error in neigh dump > >>> > >>> On 11/27/16 7:34 PM, 张胜举 wrote: > >>>>> -----Original Message----- > >>>>> From: David Ahern [mailto:dsa@cumulusnetworks.com] > >>>>> Sent: Monday, November 28, 2016 10:10 AM > >>>>> To: Zhang Shengju <zhangshengju@cmss.chinamobile.com>; > >>>>> netdev@vger.kernel.org > >>>>> Subject: Re: [net,v2] neigh: fix the loop index error in neigh > >>>>> dump > >>>>> > >>>>> On 11/27/16 6:32 PM, Zhang Shengju wrote: > >>>>>> Loop index in neigh dump function is not updated correctly under > >>>>>> some circumstances, this patch will fix it. > >>>>> > >>>>> What's an example? > >>>> > >>>> If dev is filtered out, the original code goes to next loop without > >>>> updating loop index 'idx'. > >>> > >>> And you have a use case with missing or redundant data? Or is your > >>> comment based on a review of code only? > >> It's on my code review. No use case currently, this is uncommon to > happen. > >> > >> > >>> > >>>>> You are completely rewriting the dump loops. > >>>> > >>>> I put 'idx++' into for loop, so I replace 'goto' with 'continue'. > >>>> The other change is style related. > >>> > >>> A "fixes" should not include 'style related' changes. > >> Okay, I will send another version without style changes. > >> > > > > Personally, I think you need to produce a use case that fails before sending > another patch. I have not seen a problem with this code. > > > > And looking back at 3f0ae05d6f I should not have acked it (reviewed it too > quickly while on PTO). Your change is a no-op because of what idx represents > - the position in the hash list for devices relevant for the dump request. > Same goes for the neigh dump so this patch is not needed. > No, when dump request must be processed by multiple 'recv/recvmsg' system calls, idx stores which dev/neigh the previous call have processed, so that next call will scan from the right place. So no matter whether the dev/neigh is filtered, the idx should be increased anyway. It's hard to produce a use case, because we mostly have only one entity in hash list. Even with multiple entities, we also need the function to exit right at the place where dev/neigh is filter out. All other dump functiones for RT netlink keep this logic, you can refer inet_dump_ifaddr() if you wish.
On 11/27/16 9:50 PM, 张胜举 wrote: > No, when dump request must be processed by multiple 'recv/recvmsg' system > calls, > idx stores which dev/neigh the previous call have processed, so that next > call will scan > from the right place. I have tested multiple calls and I do not see redundant information or missing information. > > So no matter whether the dev/neigh is filtered, the idx should be increased > anyway. No, it does not. Again, idx is the index in the list of devices/ of interest. It is NOT a device index nor is it the absolute index in the list. It is a relative index. The filter is the same across recvmsg calls so the idx count is absolutely fine. Produce a test case that fails.
> -----Original Message----- > From: David Ahern [mailto:dsa@cumulusnetworks.com] > Sent: Monday, November 28, 2016 1:07 PM > To: 张胜举 <zhangshengju@cmss.chinamobile.com>; > netdev@vger.kernel.org > Subject: Re: [net,v2] neigh: fix the loop index error in neigh dump > > On 11/27/16 9:50 PM, 张胜举 wrote: > > No, when dump request must be processed by multiple 'recv/recvmsg' > > system calls, idx stores which dev/neigh the previous call have > > processed, so that next call will scan from the right place. > > I have tested multiple calls and I do not see redundant information or missing > information. > > > > > So no matter whether the dev/neigh is filtered, the idx should be > > increased anyway. > > No, it does not. Again, idx is the index in the list of devices/ of interest. It is > NOT a device index nor is it the absolute index in the list. It is a relative index. > The filter is the same across recvmsg calls so the idx count is absolutely fine. > > Produce a test case that fails. David, I know your point. And I agree with you that this will not make redundant or missing link information. But this will cause the filtered out device be scanned multiple times. For example, assume that netlink message can only store two devices info. And eth2-eth5 are filtered out. For the first loop, idx will point to eth2, but the code already scan to eth6. eth0->eth1->eth2(out)->eth3(out)-> eth4(out)->eth5(out)->eth6->eth7 ^ The next loop, the code will start to scan from eth2 to eth8, but eth2-eth5 already scanned by previous loop. After this loop, idx will point to eth4. eth0->eth1->eth2(out)->eth3(out)->eth4(out)->eth5(out)->eth6->eth7->eth8 ^ So this will cause the same device to be scanned multiple times. Almost all other dump functions treat idx as the absolute index in the list, and will not have the above problem. We don't treat this a bugfix, but i think we'd better in line with other dump functions.
diff --git a/net/core/neighbour.c b/net/core/neighbour.c index 2ae929f..ce32e9c 100644 --- a/net/core/neighbour.c +++ b/net/core/neighbour.c @@ -2256,6 +2256,16 @@ static bool neigh_ifindex_filtered(struct net_device *dev, int filter_idx) return false; } +static bool neigh_dump_filtered(struct net_device *dev, int filter_idx, + int filter_master_idx) +{ + if (neigh_ifindex_filtered(dev, filter_idx) || + neigh_master_filtered(dev, filter_master_idx)) + return true; + + return false; +} + static int neigh_dump_table(struct neigh_table *tbl, struct sk_buff *skb, struct netlink_callback *cb) { @@ -2285,20 +2295,15 @@ static int neigh_dump_table(struct neigh_table *tbl, struct sk_buff *skb, rcu_read_lock_bh(); nht = rcu_dereference_bh(tbl->nht); - for (h = s_h; h < (1 << nht->hash_shift); h++) { - if (h > s_h) - s_idx = 0; + for (h = s_h; h < (1 << nht->hash_shift); h++, s_idx = 0) { for (n = rcu_dereference_bh(nht->hash_buckets[h]), idx = 0; n != NULL; - n = rcu_dereference_bh(n->next)) { - if (!net_eq(dev_net(n->dev), net)) - continue; - if (neigh_ifindex_filtered(n->dev, filter_idx)) + n = rcu_dereference_bh(n->next), idx++) { + if (idx < s_idx || !net_eq(dev_net(n->dev), net)) continue; - if (neigh_master_filtered(n->dev, filter_master_idx)) + if (neigh_dump_filtered(n->dev, filter_idx, + filter_master_idx)) continue; - if (idx < s_idx) - goto next; if (neigh_fill_info(skb, n, NETLINK_CB(cb->skb).portid, cb->nlh->nlmsg_seq, RTM_NEWNEIGH, @@ -2306,8 +2311,6 @@ static int neigh_dump_table(struct neigh_table *tbl, struct sk_buff *skb, rc = -1; goto out; } -next: - idx++; } } rc = skb->len; @@ -2328,14 +2331,10 @@ static int pneigh_dump_table(struct neigh_table *tbl, struct sk_buff *skb, read_lock_bh(&tbl->lock); - for (h = s_h; h <= PNEIGH_HASHMASK; h++) { - if (h > s_h) - s_idx = 0; - for (n = tbl->phash_buckets[h], idx = 0; n; n = n->next) { - if (pneigh_net(n) != net) + for (h = s_h; h <= PNEIGH_HASHMASK; h++, s_idx = 0) { + for (n = tbl->phash_buckets[h], idx = 0; n; n = n->next, idx++) { + if (idx < s_idx || pneigh_net(n) != net) continue; - if (idx < s_idx) - goto next; if (pneigh_fill_info(skb, n, NETLINK_CB(cb->skb).portid, cb->nlh->nlmsg_seq, RTM_NEWNEIGH, @@ -2344,8 +2343,6 @@ static int pneigh_dump_table(struct neigh_table *tbl, struct sk_buff *skb, rc = -1; goto out; } - next: - idx++; } }
Loop index in neigh dump function is not updated correctly under some circumstances, this patch will fix it. Fixes: 16660f0bd9 ("net: Add support for filtering neigh dump by device index") Fixes: 21fdd092ac ("net: Add support for filtering neigh dump by master device") Signed-off-by: Zhang Shengju <zhangshengju@cmss.chinamobile.com> --- net/core/neighbour.c | 39 ++++++++++++++++++--------------------- 1 file changed, 18 insertions(+), 21 deletions(-)