netdev - RE: [net,v2] neigh: fix the loop index error in neigh dump

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <001e01d24932$f7c0dd10$e7429730$@cmss.chinamobile.com>
Date:   Mon, 28 Nov 2016 12:50:39 +0800
From:   张胜举 <zhangshengju@...s.chinamobile.com>
To:     "'David Ahern'" <dsa@...ulusnetworks.com>, <netdev@...r.kernel.org>
Subject: RE: [net,v2] neigh: fix the loop index error in neigh dump



> -----Original Message-----
> From: David Ahern [mailto:dsa@...ulusnetworks.com]
> Sent: Monday, November 28, 2016 11:10 AM
> To: 张胜举 <zhangshengju@...s.chinamobile.com>;
> netdev@...r.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@...ulusnetworks.com]
> >>> Sent: Monday, November 28, 2016 10:39 AM
> >>> To: 张胜举 <zhangshengju@...s.chinamobile.com>;
> >>> netdev@...r.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@...ulusnetworks.com]
> >>>>> Sent: Monday, November 28, 2016 10:10 AM
> >>>>> To: Zhang Shengju <zhangshengju@...s.chinamobile.com>;
> >>>>> netdev@...r.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.