Re: NEEDS FIXING - Was: Re: [PATCH v2] ipv4: ping: fix bind address validity check

[Riccardo Paolo Bestetti]

On Sat Jun 18, 2022 at 4:58 AM CEST, Jakub Kicinski wrote:
> On Sat, 18 Jun 2022 02:32:55 +0200 Riccardo Paolo Bestetti wrote:
> > I receompiled the kernel from the net tree to do some more manual testing
> > on the patch and I have two things to disclose. Sorry for the caps in
> > the subject.
> > 
> > TL;DR: I noticed that one of the regressions tests is (correctly)
> > failing, but for the wrong reasons; and the patch I sent contains a
> > mistake, and unfortunately it has already been applied to the tree as
> > commit b4a028c4d0.
> > 
> > Long version below.
> > 
> > 1) If you run regression tests with -v, the (correct -- see below) ICMP
> > tests for broadcast and multicast binding do not fail with
> > EADDRNOTAVAIL, but with ACCES, but only when run through fcnal-test.sh.
> > This is also true for one of the additional (commented out) tests you
> > can find in my patch following this email. I'm not sure why this
> > happens; however I'm reasonably convinced it is a quirk or a consequence
> > of the testing methodology/setup. Can anyone offer any insights?
> > 
> > 2) My patch is faulty. I had a complete and tested patch, including code
> > fixing the regression. Instead of sending it, however, I decided to
> > adapt it to preserve Carlos Llamas' version of ping.c, since they posted
> > their patch first. In doing so I used a work branch which contained a
> > faulty version (wrong flags) of the regression tests. The resulting
> > faulty patch is, unfortunately, currently in the tree.
> > 
> > At this point, due to the unfortunate combination of (1) and (2), it
> > might be worth reverting the patch altogether and just applying the v1
> > (i.e. without the regression tests) to the tree and to the relevant LTS
> > versions.
>
> IIUC only the test is faulty / unreliable, correct?
>
> We have until Thursday before this patch hits Linus's tree so should 
> be plenty of time to figure the problem out and apply an incremental
> fix. I see you posted an RFC already, thanks!

I followed that up with a v2 [1] that looks good to me. Fixes the fulty
tests covering the regression, adds a couple more tests similar to what
I added back in November (but for different code paths).

As an additional note, if the regression fix (b4a028c4d0) is candidate
for the LTS, then this patch should probably be as well.

Riccardo P. Bestetti

[1]: https://patchwork.kernel.org/project/netdevbpf/patch/20220619162734.113340-1-pbl@xxxxxxxxx/

>
> > After that, a more proper discussion can be had about (1), and the
> > regression tests can be fixed. I'm sending a demonstrative patch for
> > that as a response to this message.