[PATCH net 1/5] net: netdevsim: fix inconsistent carrier state after link/unlink

yk@y-koj.net posted 5 patches 1 month, 1 week ago
There is a newer version of this series
[PATCH net 1/5] net: netdevsim: fix inconsistent carrier state after link/unlink
Posted by yk@y-koj.net 1 month, 1 week ago
From: Yohei Kojima <yk@y-koj.net>

This patch fixes the edge case behavior on ifup/ifdown and
linking/unlinking two netdevsim interfaces:

1. unlink two interfaces netdevsim1 and netdevsim2
2. ifdown netdevsim1
3. ifup netdevsim1
4. link two interfaces netdevsim1 and netdevsim2
5. (Now two interfaces are linked in terms of netdevsim peer, but
    carrier state of the two interfaces remains DOWN.)

This inconsistent behavior is caused by the current implementation,
which only cares about the "link, then ifup" order, not "ifup, then
link" order. This patch fixes the inconsistency by calling
netif_carrier_on() when two netdevsim interfaces are linked.

This patch solves buggy behavior on NetworkManager-based systems which
causes the netdevsim test to fail with the following error:

  # timeout set to 600
  # selftests: drivers/net/netdevsim: peer.sh
  # 2025/12/25 00:54:03 socat[9115] W address is opened in read-write mode but only supports read-only
  # 2025/12/25 00:56:17 socat[9115] W connect(7, AF=2 192.168.1.1:1234, 16): Connection timed out
  # 2025/12/25 00:56:17 socat[9115] E TCP:192.168.1.1:1234: Connection timed out
  # expected 3 bytes, got 0
  # 2025/12/25 00:56:17 socat[9109] W exiting on signal 15
  not ok 13 selftests: drivers/net/netdevsim: peer.sh # exit=1

This patch also fixes timeout on TCP Fast Open (TFO) test because the
test also depends on netdevsim.

Fixes: 1a8fed52f7be ("netdevsim: set the carrier when the device goes up")
Signed-off-by: Yohei Kojima <yk@y-koj.net>
---
 drivers/net/netdevsim/bus.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/netdevsim/bus.c b/drivers/net/netdevsim/bus.c
index 70e8c38ddad6..fa94c680c92a 100644
--- a/drivers/net/netdevsim/bus.c
+++ b/drivers/net/netdevsim/bus.c
@@ -332,6 +332,9 @@ static ssize_t link_device_store(const struct bus_type *bus, const char *buf, si
 	rcu_assign_pointer(nsim_a->peer, nsim_b);
 	rcu_assign_pointer(nsim_b->peer, nsim_a);
 
+	netif_carrier_on(dev_a);
+	netif_carrier_on(dev_b);
+
 out_err:
 	put_net(ns_b);
 	put_net(ns_a);
@@ -381,6 +384,9 @@ static ssize_t unlink_device_store(const struct bus_type *bus, const char *buf,
 	if (!peer)
 		goto out_put_netns;
 
+	netif_carrier_off(dev);
+	netif_carrier_off(peer->netdev);
+
 	err = 0;
 	RCU_INIT_POINTER(nsim->peer, NULL);
 	RCU_INIT_POINTER(peer->peer, NULL);
-- 
2.51.2
Re: [PATCH net 1/5] net: netdevsim: fix inconsistent carrier state after link/unlink
Posted by Andrew Lunn 1 month, 1 week ago
On Tue, Dec 30, 2025 at 03:32:34AM +0900, yk@y-koj.net wrote:
> From: Yohei Kojima <yk@y-koj.net>
> 
> This patch fixes the edge case behavior on ifup/ifdown and
> linking/unlinking two netdevsim interfaces:
> 
> 1. unlink two interfaces netdevsim1 and netdevsim2
> 2. ifdown netdevsim1
> 3. ifup netdevsim1
> 4. link two interfaces netdevsim1 and netdevsim2
> 5. (Now two interfaces are linked in terms of netdevsim peer, but
>     carrier state of the two interfaces remains DOWN.)
> 
> This inconsistent behavior is caused by the current implementation,
> which only cares about the "link, then ifup" order, not "ifup, then
> link" order. This patch fixes the inconsistency by calling
> netif_carrier_on() when two netdevsim interfaces are linked.
> 
> This patch solves buggy behavior on NetworkManager-based systems which
> causes the netdevsim test to fail with the following error:
> 
>   # timeout set to 600
>   # selftests: drivers/net/netdevsim: peer.sh
>   # 2025/12/25 00:54:03 socat[9115] W address is opened in read-write mode but only supports read-only
>   # 2025/12/25 00:56:17 socat[9115] W connect(7, AF=2 192.168.1.1:1234, 16): Connection timed out
>   # 2025/12/25 00:56:17 socat[9115] E TCP:192.168.1.1:1234: Connection timed out
>   # expected 3 bytes, got 0
>   # 2025/12/25 00:56:17 socat[9109] W exiting on signal 15
>   not ok 13 selftests: drivers/net/netdevsim: peer.sh # exit=1
> 
> This patch also fixes timeout on TCP Fast Open (TFO) test because the
> test also depends on netdevsim.
> 
> Fixes: 1a8fed52f7be ("netdevsim: set the carrier when the device goes up")

Stable rules say:

   It must either fix a real bug that bothers people or just add a device ID.

netdevsim is not a real device. Do its bugs actually bother people?
Should this patch have a Fixes: tag?

       Andrew
Re: [PATCH net 1/5] net: netdevsim: fix inconsistent carrier state after link/unlink
Posted by Yohei Kojima 1 month, 1 week ago
On Mon, Dec 29, 2025 at 07:39:29PM +0100, Andrew Lunn wrote:
> On Tue, Dec 30, 2025 at 03:32:34AM +0900, yk@y-koj.net wrote:
> > From: Yohei Kojima <yk@y-koj.net>
> > 
> > This patch fixes the edge case behavior on ifup/ifdown and
> > linking/unlinking two netdevsim interfaces:
> > 
> > 1. unlink two interfaces netdevsim1 and netdevsim2
> > 2. ifdown netdevsim1
> > 3. ifup netdevsim1
> > 4. link two interfaces netdevsim1 and netdevsim2
> > 5. (Now two interfaces are linked in terms of netdevsim peer, but
> >     carrier state of the two interfaces remains DOWN.)
> > 
> > This inconsistent behavior is caused by the current implementation,
> > which only cares about the "link, then ifup" order, not "ifup, then
> > link" order. This patch fixes the inconsistency by calling
> > netif_carrier_on() when two netdevsim interfaces are linked.
> > 
> > This patch solves buggy behavior on NetworkManager-based systems which
> > causes the netdevsim test to fail with the following error:
> > 
> >   # timeout set to 600
> >   # selftests: drivers/net/netdevsim: peer.sh
> >   # 2025/12/25 00:54:03 socat[9115] W address is opened in read-write mode but only supports read-only
> >   # 2025/12/25 00:56:17 socat[9115] W connect(7, AF=2 192.168.1.1:1234, 16): Connection timed out
> >   # 2025/12/25 00:56:17 socat[9115] E TCP:192.168.1.1:1234: Connection timed out
> >   # expected 3 bytes, got 0
> >   # 2025/12/25 00:56:17 socat[9109] W exiting on signal 15
> >   not ok 13 selftests: drivers/net/netdevsim: peer.sh # exit=1
> > 
> > This patch also fixes timeout on TCP Fast Open (TFO) test because the
> > test also depends on netdevsim.
> > 
> > Fixes: 1a8fed52f7be ("netdevsim: set the carrier when the device goes up")
> 
> Stable rules say:
> 
>    It must either fix a real bug that bothers people or just add a device ID.

Thank you for the quick reply. I don't intend for this patch to be
backported to the stable tree. My understanding was that bugfix patches
to the net tree should have Fixes: tag for historical tracking.

> 
> netdevsim is not a real device. Do its bugs actually bother people?

This patch fixes a real bug that is seen when a developer tries to test
TFO or netdevsim tests on NetworkManager-enabled systems: it causes
false positives in kselftests on such systems.

> Should this patch have a Fixes: tag?

The patch 1a8fed52f7be ("netdevsim: set the carrier when the device goes
up"), which does a similar change, has Fixes: tag. Since this patch fixes
the corner-case behavior which was missed in the previous fix, this
patch should have Fixes: tag for consistency.

> 
>        Andrew

Thank you,
Yohei Kojima
Re: [PATCH net 1/5] net: netdevsim: fix inconsistent carrier state after link/unlink
Posted by Andrew Lunn 1 month, 1 week ago
> Thank you for the quick reply. I don't intend for this patch to be
> backported to the stable tree. My understanding was that bugfix patches
> to the net tree should have Fixes: tag for historical tracking.
> 
> > 
> > netdevsim is not a real device. Do its bugs actually bother people?
> 
> This patch fixes a real bug that is seen when a developer tries to test
> TFO or netdevsim tests on NetworkManager-enabled systems: it causes
> false positives in kselftests on such systems.

O.K, then keep the Fixes tag and submit it for net. However, the tests
should be considered development work, and submitted to net-next, if
they are not fixes. Please split this into two series.

https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html

    Andrew

---
pw-bot: cr
Re: [PATCH net 1/5] net: netdevsim: fix inconsistent carrier state after link/unlink
Posted by Yohei Kojima 1 month, 1 week ago
On Tue, Dec 30, 2025 at 12:02:22PM +0100, Andrew Lunn wrote:
> > Thank you for the quick reply. I don't intend for this patch to be
> > backported to the stable tree. My understanding was that bugfix patches
> > to the net tree should have Fixes: tag for historical tracking.
> > 
> > > 
> > > netdevsim is not a real device. Do its bugs actually bother people?
> > 
> > This patch fixes a real bug that is seen when a developer tries to test
> > TFO or netdevsim tests on NetworkManager-enabled systems: it causes
> > false positives in kselftests on such systems.
> 
> O.K, then keep the Fixes tag and submit it for net. However, the tests
> should be considered development work, and submitted to net-next, if
> they are not fixes. Please split this into two series.

Sure, I've submitted the v2 patch here.

https://lore.kernel.org/netdev/cover.1767108538.git.yk@y-koj.net/

Following your suggestion, I've removed the unrelated TFO tests and
the netdevsim test improvement. I will post the removed patches as a
separate series once net-next reopens.

However, I kept the regression test for this patch in the v2 series, as
the "1.5.10. Co-posting selftests" section in the maintainer-netdev
document says:

  Selftests should be part of the same series as the code changes.
  Specifically for fixes both code change and related test should
  go into the same tree (the tests may lack a Fixes tag, which is
  expected). Mixing code changes and test changes in a single commit
  is discouraged.

> 
> https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html
> 
>     Andrew

Thank you,
Yohei Kojima

> 
> ---
> pw-bot: cr
Re: [PATCH net 1/5] net: netdevsim: fix inconsistent carrier state after link/unlink
Posted by Andrew Lunn 1 month, 1 week ago
> Sure, I've submitted the v2 patch here.
> 
> https://lore.kernel.org/netdev/cover.1767108538.git.yk@y-koj.net/
> 
> Following your suggestion, I've removed the unrelated TFO tests and
> the netdevsim test improvement. I will post the removed patches as a
> separate series once net-next reopens.
> 
> However, I kept the regression test for this patch in the v2 series, as
> the "1.5.10. Co-posting selftests" section in the maintainer-netdev
> document says:

Thanks for doing this. I don't know enough about netdevsim to be able
to do a proper review, so i will let somebody else do that.

   Andrew
Re: [PATCH net 1/5] net: netdevsim: fix inconsistent carrier state after link/unlink
Posted by Yohei Kojima 1 month, 1 week ago
On Tue, Dec 30, 2025 at 07:38:01PM +0100, Andrew Lunn wrote:
> > Sure, I've submitted the v2 patch here.
> > 
> > https://lore.kernel.org/netdev/cover.1767108538.git.yk@y-koj.net/
> > 
> > Following your suggestion, I've removed the unrelated TFO tests and
> > the netdevsim test improvement. I will post the removed patches as a
> > separate series once net-next reopens.
> > 
> > However, I kept the regression test for this patch in the v2 series, as
> > the "1.5.10. Co-posting selftests" section in the maintainer-netdev
> > document says:
> 
> Thanks for doing this. I don't know enough about netdevsim to be able
> to do a proper review, so i will let somebody else do that.

Okay, thank you for the early review!

> 
>    Andrew

Best regards,
Yohei Kojima