On 12/07/2023 17:48, Ido Schimmel wrote:
> On Tue, Jul 11, 2023 at 04:54:15PM -0700, Kuniyuki Iwashima wrote:
>> When we create an L2 loop on a bridge in netns, we will see packets storm
>> even if STP is enabled.
>>
>> # unshare -n
>> # ip link add br0 type bridge
>> # ip link add veth0 type veth peer name veth1
>> # ip link set veth0 master br0 up
>> # ip link set veth1 master br0 up
>> # ip link set br0 type bridge stp_state 1
>> # ip link set br0 up
>> # sleep 30
>> # ip -s link show br0
>> 2: br0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP mode DEFAULT group default qlen 1000
>> link/ether b6:61:98:1c:1c:b5 brd ff:ff:ff:ff:ff:ff
>> RX: bytes packets errors dropped missed mcast
>> 956553768 12861249 0 0 0 12861249 <-. Keep
>> TX: bytes packets errors dropped carrier collsns | increasing
>> 1027834 11951 0 0 0 0 <-' rapidly
>>
>> This is because llc_rcv() drops all packets in non-root netns and BPDU
>> is dropped.
>>
>> Let's show an error when enabling STP in netns.
>>
>> # unshare -n
>> # ip link add br0 type bridge
>> # ip link set br0 type bridge stp_state 1
>> Error: bridge: STP can't be enabled in non-root netns.
>>
>> Note this commit will be reverted later when we namespacify the whole LLC
>> infra.
>>
>> Fixes: e730c15519d0 ("[NET]: Make packet reception network namespace safe")
>> Suggested-by: Harry Coin <hcoin@xxxxxxxxxxxxxxxxx>
>
> I'm not sure that's accurate. I read his response in the link below and
> he says "I'd rather be warned than blocked" and "But better warned and
> awaiting a fix than blocked", which I agree with. The patch has the
> potential to cause a lot of regressions, but without actually fixing the
> problem.
>
> How about simply removing the error [1]? Since iproute2 commit
> 844c37b42373 ("libnetlink: Handle extack messages for non-error case"),
> it can print extack warnings and not only errors. With the diff below:
>
> # unshare -n
> # ip link add name br0 type bridge
> # ip link set dev br0 type bridge stp_state 1
> Warning: bridge: STP can't be enabled in non-root netns.
> # echo $?
> 0
>
> [1]
> diff --git a/net/bridge/br_stp_if.c b/net/bridge/br_stp_if.c
> index a807996ac56b..b5143de37938 100644
> --- a/net/bridge/br_stp_if.c
> +++ b/net/bridge/br_stp_if.c
> @@ -201,10 +201,8 @@ int br_stp_set_enabled(struct net_bridge *br, unsigned long val,
> {
> ASSERT_RTNL();
>
> - if (!net_eq(dev_net(br->dev), &init_net)) {
> + if (!net_eq(dev_net(br->dev), &init_net))
> NL_SET_ERR_MSG_MOD(extack, "STP can't be enabled in non-root netns");
> - return -EINVAL;
> - }
>
> if (br_mrp_enabled(br)) {
> NL_SET_ERR_MSG_MOD(extack,
>
I'd prefer this approach to changing user-visible behaviour and potential regressions.
Just change the warning message.
Thanks,
Nik
