2022-06-17 09:05:43

by Riccardo Paolo Bestetti

[permalink] [raw]
Subject: [PATCH v2] ipv4: ping: fix bind address validity check

Commit 8ff978b8b222 ("ipv4/raw: support binding to nonlocal addresses")
introduced a helper function to fold duplicated validity checks of bind
addresses into inet_addr_valid_or_nonlocal(). However, this caused an
unintended regression in ping_check_bind_addr(), which previously would
reject binding to multicast and broadcast addresses, but now these are
both incorrectly allowed as reported in [1].

This patch restores the original check. A simple reordering is done to
improve readability and make it evident that multicast and broadcast
addresses should not be allowed. Also, add an early exit for INADDR_ANY
which replaces lost behavior added by commit 0ce779a9f501 ("net: Avoid
unnecessary inet_addr_type() call when addr is INADDR_ANY").

Furthermore, this patch introduces regression selftests to catch these
specific cases.

[1] https://lore.kernel.org/netdev/CANP3RGdkAcDyAZoT1h8Gtuu0saq+eOrrTiWbxnOs+5zn+cpyKg@mail.gmail.com/

Fixes: 8ff978b8b222 ("ipv4/raw: support binding to nonlocal addresses")
Cc: Miaohe Lin <[email protected]>
Reported-by: Maciej Żenczykowski <[email protected]>
Signed-off-by: Carlos Llamas <[email protected]>
Signed-off-by: Riccardo Paolo Bestetti <[email protected]>
---
This patch is sent as a follow-up to the discussion on the v1 by Carlos
Llamas.

Original thread:
https://lore.kernel.org/netdev/[email protected]/

net/ipv4/ping.c | 10 ++++---
tools/testing/selftests/net/fcnal-test.sh | 33 +++++++++++++++++++++++
2 files changed, 40 insertions(+), 3 deletions(-)

diff --git a/net/ipv4/ping.c b/net/ipv4/ping.c
index 1a43ca73f94d..3c6101def7d6 100644
--- a/net/ipv4/ping.c
+++ b/net/ipv4/ping.c
@@ -319,12 +319,16 @@ static int ping_check_bind_addr(struct sock *sk, struct inet_sock *isk,
pr_debug("ping_check_bind_addr(sk=%p,addr=%pI4,port=%d)\n",
sk, &addr->sin_addr.s_addr, ntohs(addr->sin_port));

+ if (addr->sin_addr.s_addr == htonl(INADDR_ANY))
+ return 0;
+
tb_id = l3mdev_fib_table_by_index(net, sk->sk_bound_dev_if) ? : tb_id;
chk_addr_ret = inet_addr_type_table(net, addr->sin_addr.s_addr, tb_id);

- if (!inet_addr_valid_or_nonlocal(net, inet_sk(sk),
- addr->sin_addr.s_addr,
- chk_addr_ret))
+ if (chk_addr_ret == RTN_MULTICAST ||
+ chk_addr_ret == RTN_BROADCAST ||
+ (chk_addr_ret != RTN_LOCAL &&
+ !inet_can_nonlocal_bind(net, isk)))
return -EADDRNOTAVAIL;

#if IS_ENABLED(CONFIG_IPV6)
diff --git a/tools/testing/selftests/net/fcnal-test.sh b/tools/testing/selftests/net/fcnal-test.sh
index 54701c8b0cd7..75223b63e3c8 100755
--- a/tools/testing/selftests/net/fcnal-test.sh
+++ b/tools/testing/selftests/net/fcnal-test.sh
@@ -70,6 +70,10 @@ NSB_LO_IP6=2001:db8:2::2
NL_IP=172.17.1.1
NL_IP6=2001:db8:4::1

+# multicast and broadcast addresses
+MCAST_IP=224.0.0.1
+BCAST_IP=255.255.255.255
+
MD5_PW=abc123
MD5_WRONG_PW=abc1234

@@ -308,6 +312,9 @@ addr2str()
127.0.0.1) echo "loopback";;
::1) echo "IPv6 loopback";;

+ ${BCAST_IP}) echo "broadcast";;
+ ${MCAST_IP}) echo "multicast";;
+
${NSA_IP}) echo "ns-A IP";;
${NSA_IP6}) echo "ns-A IPv6";;
${NSA_LO_IP}) echo "ns-A loopback IP";;
@@ -1800,6 +1807,19 @@ ipv4_addr_bind_novrf()
run_cmd nettest -s -R -P icmp -f -l ${a} -I ${NSA_DEV} -b
log_test_addr ${a} $? 0 "Raw socket bind to nonlocal address after device bind"

+ #
+ # check that ICMP sockets cannot bind to broadcast and multicast addresses
+ #
+ a=${BCAST_IP}
+ log_start
+ run_cmd nettest -s -R -P icmp -l ${a} -b
+ log_test_addr ${a} $? 1 "ICMP socket bind to broadcast address"
+
+ a=${MCAST_IP}
+ log_start
+ run_cmd nettest -s -R -P icmp -f -l ${a} -b
+ log_test_addr ${a} $? 1 "ICMP socket bind to multicast address"
+
#
# tcp sockets
#
@@ -1857,6 +1877,19 @@ ipv4_addr_bind_vrf()
run_cmd nettest -s -R -P icmp -f -l ${a} -I ${VRF} -b
log_test_addr ${a} $? 0 "Raw socket bind to nonlocal address after VRF bind"

+ #
+ # check that ICMP sockets cannot bind to broadcast and multicast addresses
+ #
+ a=${BCAST_IP}
+ log_start
+ run_cmd nettest -s -R -P icmp -l ${a} -I ${VRF} -b
+ log_test_addr ${a} $? 1 "ICMP socket bind to broadcast address after VRF bind"
+
+ a=${MCAST_IP}
+ log_start
+ run_cmd nettest -s -R -P icmp -f -l ${a} -I ${VRF} -b
+ log_test_addr ${a} $? 1 "ICMP socket bind to multicast address after VRF bind"
+
#
# tcp sockets
#
--
2.36.1


2022-06-17 11:58:57

by patchwork-bot+netdevbpf

[permalink] [raw]
Subject: Re: [PATCH v2] ipv4: ping: fix bind address validity check

Hello:

This patch was applied to netdev/net.git (master)
by David S. Miller <[email protected]>:

On Fri, 17 Jun 2022 10:54:35 +0200 you wrote:
> Commit 8ff978b8b222 ("ipv4/raw: support binding to nonlocal addresses")
> introduced a helper function to fold duplicated validity checks of bind
> addresses into inet_addr_valid_or_nonlocal(). However, this caused an
> unintended regression in ping_check_bind_addr(), which previously would
> reject binding to multicast and broadcast addresses, but now these are
> both incorrectly allowed as reported in [1].
>
> [...]

Here is the summary with links:
- [v2] ipv4: ping: fix bind address validity check
https://git.kernel.org/netdev/net/c/b4a028c4d031

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html


2022-06-17 17:08:20

by Carlos Llamas

[permalink] [raw]
Subject: Re: [PATCH v2] ipv4: ping: fix bind address validity check

On Fri, Jun 17, 2022 at 10:54:35AM +0200, Riccardo Paolo Bestetti wrote:
> Commit 8ff978b8b222 ("ipv4/raw: support binding to nonlocal addresses")
> introduced a helper function to fold duplicated validity checks of bind
> addresses into inet_addr_valid_or_nonlocal(). However, this caused an
> unintended regression in ping_check_bind_addr(), which previously would
> reject binding to multicast and broadcast addresses, but now these are
> both incorrectly allowed as reported in [1].
>
> This patch restores the original check. A simple reordering is done to
> improve readability and make it evident that multicast and broadcast
> addresses should not be allowed. Also, add an early exit for INADDR_ANY
> which replaces lost behavior added by commit 0ce779a9f501 ("net: Avoid
> unnecessary inet_addr_type() call when addr is INADDR_ANY").
>
> Furthermore, this patch introduces regression selftests to catch these
> specific cases.
>
> [1] https://lore.kernel.org/netdev/CANP3RGdkAcDyAZoT1h8Gtuu0saq+eOrrTiWbxnOs+5zn+cpyKg@mail.gmail.com/
>
> Fixes: 8ff978b8b222 ("ipv4/raw: support binding to nonlocal addresses")
> Cc: Miaohe Lin <[email protected]>
> Reported-by: Maciej Żenczykowski <[email protected]>
> Signed-off-by: Carlos Llamas <[email protected]>
> Signed-off-by: Riccardo Paolo Bestetti <[email protected]>
> ---
> This patch is sent as a follow-up to the discussion on the v1 by Carlos
> Llamas.
>
> Original thread:
> https://lore.kernel.org/netdev/[email protected]/
>

Reviewed-by: Carlos Llamas <[email protected]>

Thanks Riccardo for adding the test cases. I would appreciate it if next
time you add a co-developed tag or maybe a separate commit as opposed to
overriding the original author. This is fine though.

--
Carlos Llamas

2022-06-17 18:41:19

by Riccardo Paolo Bestetti

[permalink] [raw]
Subject: Re: [PATCH v2] ipv4: ping: fix bind address validity check

On Fri Jun 17, 2022 at 6:39 PM CEST, Carlos Llamas wrote:
> Thanks Riccardo for adding the test cases. I would appreciate it if next
> time you add a co-developed tag or maybe a separate commit as opposed to
> overriding the original author. This is fine though.

For context, I had the whole patch ready since a few days ago. This
morning I woke up to the v1, so I decided to just apply my tests and
send it off as a v2.

I evidently forgot to add the Co-developed-by in the process, for which
I apologize. I wish that could be fixed, but the patch was almost
immediately applied to the tree. (I stand by the choice of having a
single commit for the fix, however.)

Riccardo P. Bestetti

2022-06-17 23:57:21

by Maciej Żenczykowski

[permalink] [raw]
Subject: Re: [PATCH v2] ipv4: ping: fix bind address validity check

On Fri, Jun 17, 2022 at 4:30 AM <[email protected]> wrote:
>
> Hello:
>
> This patch was applied to netdev/net.git (master)
> by David S. Miller <[email protected]>:
>
> On Fri, 17 Jun 2022 10:54:35 +0200 you wrote:
> > Commit 8ff978b8b222 ("ipv4/raw: support binding to nonlocal addresses")
> > introduced a helper function to fold duplicated validity checks of bind
> > addresses into inet_addr_valid_or_nonlocal(). However, this caused an
> > unintended regression in ping_check_bind_addr(), which previously would
> > reject binding to multicast and broadcast addresses, but now these are
> > both incorrectly allowed as reported in [1].
> >
> > [...]
>
> Here is the summary with links:
> - [v2] ipv4: ping: fix bind address validity check
> https://git.kernel.org/netdev/net/c/b4a028c4d031
>
> You are awesome, thank you!
> --
> Deet-doot-dot, I am a bot.
> https://korg.docs.kernel.org/patchwork/pwbot.html

I believe this [
https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=b4a028c4d031
] needs to end up in 5.17+ LTS (though I guess 5.17 is eol, so
probably just 5.18)

2022-06-18 00:45:35

by Riccardo Paolo Bestetti

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

On Fri Jun 17, 2022 at 1:30 PM CEST, wrote:
> Hello:
>
> This patch was applied to netdev/net.git (master)
> by David S. Miller <[email protected]>:
>
> On Fri, 17 Jun 2022 10:54:35 +0200 you wrote:
> > Commit 8ff978b8b222 ("ipv4/raw: support binding to nonlocal addresses")
> > introduced a helper function to fold duplicated validity checks of bind
> > addresses into inet_addr_valid_or_nonlocal(). However, this caused an
> > unintended regression in ping_check_bind_addr(), which previously would
> > reject binding to multicast and broadcast addresses, but now these are
> > both incorrectly allowed as reported in [1].
> >
> > [...]
>
> Here is the summary with links:
> - [v2] ipv4: ping: fix bind address validity check
> https://git.kernel.org/netdev/net/c/b4a028c4d031
>
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.

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.

Riccardo P. Bestetti



> You are awesome, thank you!
> --
> Deet-doot-dot, I am a bot.
> https://korg.docs.kernel.org/patchwork/pwbot.html

2022-06-18 01:07:26

by Riccardo Paolo Bestetti

[permalink] [raw]
Subject: [RFC PATCH net] ipv4: fix bind address validity regression tests

Commit 8ff978b8b222 ("ipv4/raw: support binding to nonlocal addresses")
introduced support for binding to nonlocal addresses, as well as some
basic test coverage for some of the cases.

Commit b4a028c4d031 ("ipv4: ping: fix bind address validity check")
fixes a regression which incorrectly removed some checks for bind
address validation. In addition, it introduces regression tests for
those specific checks. However, those regression tests are defective, in
that they perform the tests using an incorrect combination of bind
flags. As a result, those tests fail when they should succeed.

This commit introduces additional regression tests for nonlocal binding
and fixes the defective regression tests.

PLEASE NOTE THAT THIS PATCH SHOULD NOT BE APPLIED AS-IS. The ICMP
broadcast and multicast regression tests succeed, but they do so while
returning the wrong error status. In particular, it isn't the bind that
fails, but the socket creation. This is /not/ correct, and it must be
investigated to have proper regression testing. Other instances where
this happens are: 1) if the broadcast/multicast addresses are replace
with an allowed (e.g. local) address (bind should work, but socket is
never created in the first place); 2) the commented out tests (nonlocal
bind should work but ditto.) Additionally, please note that when the
test cases are manually (i.e. without the network namespace setup from
fcnal-test.sh) ran, the expected/correct outcome is observed. The reason
I'm submitting this patch for comments, is that I'm failing to
understand where the issue lies. (Disclamer: might be something
stupid/trivial that I'm plainly missing due to tunnel vision.)

Signed-off-by: Riccardo Paolo Bestetti <[email protected]>
---
tools/testing/selftests/net/fcnal-test.sh | 36 +++++++++++++++++------
1 file changed, 27 insertions(+), 9 deletions(-)

diff --git a/tools/testing/selftests/net/fcnal-test.sh b/tools/testing/selftests/net/fcnal-test.sh
index 75223b63e3c8..778288539879 100755
--- a/tools/testing/selftests/net/fcnal-test.sh
+++ b/tools/testing/selftests/net/fcnal-test.sh
@@ -1800,24 +1800,33 @@ ipv4_addr_bind_novrf()
done

#
- # raw socket with nonlocal bind
+ # tests for nonlocal bind
#
a=${NL_IP}
log_start
- run_cmd nettest -s -R -P icmp -f -l ${a} -I ${NSA_DEV} -b
- log_test_addr ${a} $? 0 "Raw socket bind to nonlocal address after device bind"
+ run_cmd nettest -s -R -f -l ${a} -b
+ log_test_addr ${a} $? 0 "Raw socket bind to nonlocal address"
+
+ log_start
+ run_cmd nettest -s -f -l ${a} -b
+ log_test_addr ${a} $? 0 "TCP socket bind to nonlocal address"
+
+ # currently fails with ACCES
+ #log_start
+ #run_cmd nettest -s -D -P icmp -f -l ${a} -b
+ #log_test_addr ${a} $? 0 "ICMP socket bind to nonlocal address"

#
# check that ICMP sockets cannot bind to broadcast and multicast addresses
#
a=${BCAST_IP}
log_start
- run_cmd nettest -s -R -P icmp -l ${a} -b
+ run_cmd nettest -s -D -P icmp -l ${a} -b
log_test_addr ${a} $? 1 "ICMP socket bind to broadcast address"

a=${MCAST_IP}
log_start
- run_cmd nettest -s -R -P icmp -f -l ${a} -b
+ run_cmd nettest -s -D -P icmp -l ${a} -b
log_test_addr ${a} $? 1 "ICMP socket bind to multicast address"

#
@@ -1870,24 +1879,33 @@ ipv4_addr_bind_vrf()
log_test_addr ${a} $? 1 "Raw socket bind to out of scope address after VRF bind"

#
- # raw socket with nonlocal bind
+ # tests for nonlocal bind
#
a=${NL_IP}
log_start
- run_cmd nettest -s -R -P icmp -f -l ${a} -I ${VRF} -b
+ run_cmd nettest -s -R -f -l ${a} -I ${VRF} -b
log_test_addr ${a} $? 0 "Raw socket bind to nonlocal address after VRF bind"

+ log_start
+ run_cmd nettest -s -f -l ${a} -I ${VRF} -b
+ log_test_addr ${a} $? 0 "TCP socket bind to nonlocal address after VRF bind"
+
+ # currently fails with ACCES
+ #log_start
+ #run_cmd nettest -s -D -P icmp -f -l ${a} -I ${VRF} -b
+ #log_test_addr ${a} $? 0 "ICMP socket bind to nonlocal address after VRF bind"
+
#
# check that ICMP sockets cannot bind to broadcast and multicast addresses
#
a=${BCAST_IP}
log_start
- run_cmd nettest -s -R -P icmp -l ${a} -I ${VRF} -b
+ run_cmd nettest -s -D -P icmp -l ${a} -I ${VRF} -b
log_test_addr ${a} $? 1 "ICMP socket bind to broadcast address after VRF bind"

a=${MCAST_IP}
log_start
- run_cmd nettest -s -R -P icmp -f -l ${a} -I ${VRF} -b
+ run_cmd nettest -s -D -P icmp -l ${a} -I ${VRF} -b
log_test_addr ${a} $? 1 "ICMP socket bind to multicast address after VRF bind"

#
--
2.36.1

2022-06-18 03:23:39

by Jakub Kicinski

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

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!

> 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.

2022-06-18 08:12:45

by Riccardo Paolo Bestetti

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

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:
> > [...]
> > 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?

Correct, I don't see anything wrong with the fix itself. When manually
tested it shows the intended behaviour (sorry for the long lines):

$ pwd
$KERNEL_TREE/net/tools/testing/selftests/net

$ uname -a
Linux enhorning 5.19.0-rc2kbr-00103-gb4a028c4d031 #1 SMP PREEMPT_DYNAMIC Fri Jun 17 13:51:24 CEST 2022 x86_64 GNU/Linux

$ ./nettest -s -D -P icmp -l 224.0.0.1 -b
09:19:50 server: error binding socket: 99: Cannot assign requested address

$ ./nettest -s -D -P icmp -l 255.255.255.255 -b
09:21:20 server: error binding socket: 99: Cannot assign requested address

(and fcnal-test.sh shows nothing noteworthy.)


Riccardo P. Bestetti

>
> 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!
>
> > 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.

2022-06-19 17:54:09

by Riccardo Paolo Bestetti

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

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/[email protected]/

>
> > 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.

2022-06-20 10:21:35

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [PATCH v2] ipv4: ping: fix bind address validity check

On Fri, Jun 17, 2022 at 04:45:52PM -0700, Maciej Żenczykowski wrote:
> On Fri, Jun 17, 2022 at 4:30 AM <[email protected]> wrote:
> >
> > Hello:
> >
> > This patch was applied to netdev/net.git (master)
> > by David S. Miller <[email protected]>:
> >
> > On Fri, 17 Jun 2022 10:54:35 +0200 you wrote:
> > > Commit 8ff978b8b222 ("ipv4/raw: support binding to nonlocal addresses")
> > > introduced a helper function to fold duplicated validity checks of bind
> > > addresses into inet_addr_valid_or_nonlocal(). However, this caused an
> > > unintended regression in ping_check_bind_addr(), which previously would
> > > reject binding to multicast and broadcast addresses, but now these are
> > > both incorrectly allowed as reported in [1].
> > >
> > > [...]
> >
> > Here is the summary with links:
> > - [v2] ipv4: ping: fix bind address validity check
> > https://git.kernel.org/netdev/net/c/b4a028c4d031
> >
> > You are awesome, thank you!
> > --
> > Deet-doot-dot, I am a bot.
> > https://korg.docs.kernel.org/patchwork/pwbot.html
>
> I believe this [
> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=b4a028c4d031
> ] needs to end up in 5.17+ LTS (though I guess 5.17 is eol, so
> probably just 5.18)

5.17 is end-of-life, sorry.

And this needs to hit Linus's tree first.

thanks,

greg k-h

2022-06-23 19:20:39

by Maciej Żenczykowski

[permalink] [raw]
Subject: Re: [PATCH v2] ipv4: ping: fix bind address validity check

On Mon, Jun 20, 2022 at 3:11 AM Greg KH <[email protected]> wrote:
> On Fri, Jun 17, 2022 at 04:45:52PM -0700, Maciej Żenczykowski wrote:
> > On Fri, Jun 17, 2022 at 4:30 AM <[email protected]> wrote:
> > >
> > > Hello:
> > >
> > > This patch was applied to netdev/net.git (master)
> > > by David S. Miller <[email protected]>:
> > >
> > > On Fri, 17 Jun 2022 10:54:35 +0200 you wrote:
> > > > Commit 8ff978b8b222 ("ipv4/raw: support binding to nonlocal addresses")
> > > > introduced a helper function to fold duplicated validity checks of bind
> > > > addresses into inet_addr_valid_or_nonlocal(). However, this caused an
> > > > unintended regression in ping_check_bind_addr(), which previously would
> > > > reject binding to multicast and broadcast addresses, but now these are
> > > > both incorrectly allowed as reported in [1].
> > > >
> > > > [...]
> > >
> > > Here is the summary with links:
> > > - [v2] ipv4: ping: fix bind address validity check
> > > https://git.kernel.org/netdev/net/c/b4a028c4d031
> > >
> > > You are awesome, thank you!
> > > --
> > > Deet-doot-dot, I am a bot.
> > > https://korg.docs.kernel.org/patchwork/pwbot.html
> >
> > I believe this [
> > https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=b4a028c4d031
> > ] needs to end up in 5.17+ LTS (though I guess 5.17 is eol, so
> > probably just 5.18)
>
> 5.17 is end-of-life, sorry.
>
> And this needs to hit Linus's tree first.

It now has:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/net/ipv4/ping.c

ipv4: ping: fix bind address validity check
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/net/ipv4/ping.c?id=b4a028c4d031c27704ad73b1195ca69a1206941e

> thanks,
>
> greg k-h

Thanks,
Maciej

2022-06-25 14:28:09

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [PATCH v2] ipv4: ping: fix bind address validity check

On Thu, Jun 23, 2022 at 11:18:21AM -0700, Maciej Żenczykowski wrote:
> On Mon, Jun 20, 2022 at 3:11 AM Greg KH <[email protected]> wrote:
> > On Fri, Jun 17, 2022 at 04:45:52PM -0700, Maciej Żenczykowski wrote:
> > > On Fri, Jun 17, 2022 at 4:30 AM <[email protected]> wrote:
> > > >
> > > > Hello:
> > > >
> > > > This patch was applied to netdev/net.git (master)
> > > > by David S. Miller <[email protected]>:
> > > >
> > > > On Fri, 17 Jun 2022 10:54:35 +0200 you wrote:
> > > > > Commit 8ff978b8b222 ("ipv4/raw: support binding to nonlocal addresses")
> > > > > introduced a helper function to fold duplicated validity checks of bind
> > > > > addresses into inet_addr_valid_or_nonlocal(). However, this caused an
> > > > > unintended regression in ping_check_bind_addr(), which previously would
> > > > > reject binding to multicast and broadcast addresses, but now these are
> > > > > both incorrectly allowed as reported in [1].
> > > > >
> > > > > [...]
> > > >
> > > > Here is the summary with links:
> > > > - [v2] ipv4: ping: fix bind address validity check
> > > > https://git.kernel.org/netdev/net/c/b4a028c4d031
> > > >
> > > > You are awesome, thank you!
> > > > --
> > > > Deet-doot-dot, I am a bot.
> > > > https://korg.docs.kernel.org/patchwork/pwbot.html
> > >
> > > I believe this [
> > > https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=b4a028c4d031
> > > ] needs to end up in 5.17+ LTS (though I guess 5.17 is eol, so
> > > probably just 5.18)
> >
> > 5.17 is end-of-life, sorry.
> >
> > And this needs to hit Linus's tree first.
>
> It now has:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/net/ipv4/ping.c
>
> ipv4: ping: fix bind address validity check
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/net/ipv4/ping.c?id=b4a028c4d031c27704ad73b1195ca69a1206941e

Great, now queued up, thanks.

greg k-h