Currently adding a bridge vlan to a port only checks for an 8021q upper
of that vlan on the port, but does not check for matching 8021q uppers
on other ports.
This leads to the possibility of configuring shared vlans on ports after
adding uppers.
E.g. adding the upper after configuring the vlan would be rejected
$ ip link add br0 type bridge vlan filtering 1
$ ip link set swp1 master br0
$ ip link set swp2 master br0
$ bridge vlan add dev swp2 vid 100
$ ip link add swp1.100 link swp1 type vlan id 100
RTNETLINK answers: Resource busy
But the other way around would currently be accepted:
$ ip link add br0 type bridge vlan filtering 1
$ ip link set swp1 master br0
$ ip link set swp2 master br0
$ ip link add swp1.100 link swp1 type vlan id 100
$ bridge vlan add dev swp2 vid 100
$ bridge vlan
port vlan-id
swp2 1 PVID Egress Untagged
100
swp1 1 PVID Egress Untagged
br0 1 PVID Egress Untagged
Fix this by checking all members of the bridge for a matching vlan
upper, and not the port itself.
After:
$ ip link add br0 type bridge vlan filtering 1
$ ip link set swp1 master br0
$ ip link set swp2 master br0
$ ip link add swp1.100 link swp1 type vlan id 100
$ bridge vlan add dev swp2 vid 100
RTNETLINK answers: Resource busy
Fixes: 1ce39f0ee8da ("net: dsa: convert denying bridge VLAN with existing 8021q upper to PRECHANGEUPPER")
Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com>
---
v1 -> v2:
* no changes
net/dsa/user.c | 31 ++++++++++++++++++++-----------
1 file changed, 20 insertions(+), 11 deletions(-)
diff --git a/net/dsa/user.c b/net/dsa/user.c
index f59d66f0975d..fa1fe0f1493a 100644
--- a/net/dsa/user.c
+++ b/net/dsa/user.c
@@ -653,21 +653,30 @@ static int dsa_user_port_attr_set(struct net_device *dev, const void *ctx,
/* Must be called under rcu_read_lock() */
static int
-dsa_user_vlan_check_for_8021q_uppers(struct net_device *user,
+dsa_user_vlan_check_for_8021q_uppers(struct dsa_port *dp,
const struct switchdev_obj_port_vlan *vlan)
{
- struct net_device *upper_dev;
- struct list_head *iter;
+ struct dsa_switch *ds = dp->ds;
+ struct dsa_port *other_dp;
- netdev_for_each_upper_dev_rcu(user, upper_dev, iter) {
- u16 vid;
+ dsa_switch_for_each_user_port(other_dp, ds) {
+ struct net_device *user = other_dp->user;
+ struct net_device *upper_dev;
+ struct list_head *iter;
- if (!is_vlan_dev(upper_dev))
+ if (!dsa_port_bridge_same(dp, other_dp))
continue;
- vid = vlan_dev_vlan_id(upper_dev);
- if (vid == vlan->vid)
- return -EBUSY;
+ netdev_for_each_upper_dev_rcu(user, upper_dev, iter) {
+ u16 vid;
+
+ if (!is_vlan_dev(upper_dev))
+ continue;
+
+ vid = vlan_dev_vlan_id(upper_dev);
+ if (vid == vlan->vid)
+ return -EBUSY;
+ }
}
return 0;
@@ -693,11 +702,11 @@ static int dsa_user_vlan_add(struct net_device *dev,
*/
if (br_vlan_enabled(dsa_port_bridge_dev_get(dp))) {
rcu_read_lock();
- err = dsa_user_vlan_check_for_8021q_uppers(dev, vlan);
+ err = dsa_user_vlan_check_for_8021q_uppers(dp, vlan);
rcu_read_unlock();
if (err) {
NL_SET_ERR_MSG_MOD(extack,
- "Port already has a VLAN upper with this VID");
+ "This VLAN already has an upper configured on a bridge port");
return err;
}
}
--
2.43.0On Mon, Dec 01, 2025 at 11:28:13AM +0100, Jonas Gorski wrote:
...
> diff --git a/net/dsa/user.c b/net/dsa/user.c
> index f59d66f0975d..fa1fe0f1493a 100644
> --- a/net/dsa/user.c
> +++ b/net/dsa/user.c
> @@ -653,21 +653,30 @@ static int dsa_user_port_attr_set(struct net_device *dev, const void *ctx,
>
> /* Must be called under rcu_read_lock() */
> static int
> -dsa_user_vlan_check_for_8021q_uppers(struct net_device *user,
> +dsa_user_vlan_check_for_8021q_uppers(struct dsa_port *dp,
> const struct switchdev_obj_port_vlan *vlan)
> {
> - struct net_device *upper_dev;
> - struct list_head *iter;
> + struct dsa_switch *ds = dp->ds;
> + struct dsa_port *other_dp;
>
> - netdev_for_each_upper_dev_rcu(user, upper_dev, iter) {
> - u16 vid;
> + dsa_switch_for_each_user_port(other_dp, ds) {
> + struct net_device *user = other_dp->user;
Hi Jonas,
The AI robot is concerned that user may be NULL here.
And I can't convince myself that cannot be the case.
Could you take a look?
https://netdev-ai.bots.linux.dev/ai-review.html?id=3d47057e-e740-4b66-9d60-9ec2a7ee92a1#patch-0
> + struct net_device *upper_dev;
> + struct list_head *iter;
>
> - if (!is_vlan_dev(upper_dev))
> + if (!dsa_port_bridge_same(dp, other_dp))
> continue;
>
> - vid = vlan_dev_vlan_id(upper_dev);
> - if (vid == vlan->vid)
> - return -EBUSY;
> + netdev_for_each_upper_dev_rcu(user, upper_dev, iter) {
> + u16 vid;
> +
> + if (!is_vlan_dev(upper_dev))
> + continue;
> +
> + vid = vlan_dev_vlan_id(upper_dev);
> + if (vid == vlan->vid)
> + return -EBUSY;
> + }
> }
>
> return 0;
...
Hi,
On Mon, Dec 1, 2025 at 3:48 PM Simon Horman <horms@kernel.org> wrote:
>
> On Mon, Dec 01, 2025 at 11:28:13AM +0100, Jonas Gorski wrote:
>
> ...
>
> > diff --git a/net/dsa/user.c b/net/dsa/user.c
> > index f59d66f0975d..fa1fe0f1493a 100644
> > --- a/net/dsa/user.c
> > +++ b/net/dsa/user.c
> > @@ -653,21 +653,30 @@ static int dsa_user_port_attr_set(struct net_device *dev, const void *ctx,
> >
> > /* Must be called under rcu_read_lock() */
> > static int
> > -dsa_user_vlan_check_for_8021q_uppers(struct net_device *user,
> > +dsa_user_vlan_check_for_8021q_uppers(struct dsa_port *dp,
> > const struct switchdev_obj_port_vlan *vlan)
> > {
> > - struct net_device *upper_dev;
> > - struct list_head *iter;
> > + struct dsa_switch *ds = dp->ds;
> > + struct dsa_port *other_dp;
> >
> > - netdev_for_each_upper_dev_rcu(user, upper_dev, iter) {
> > - u16 vid;
> > + dsa_switch_for_each_user_port(other_dp, ds) {
> > + struct net_device *user = other_dp->user;
>
> Hi Jonas,
>
> The AI robot is concerned that user may be NULL here.
> And I can't convince myself that cannot be the case.
>
> Could you take a look?
>
> https://netdev-ai.bots.linux.dev/ai-review.html?id=3d47057e-e740-4b66-9d60-9ec2a7ee92a1#patch-0
At this point it can be NULL. But it being NULL is not an issue, as ...
>
> > + struct net_device *upper_dev;
> > + struct list_head *iter;
> >
> > - if (!is_vlan_dev(upper_dev))
> > + if (!dsa_port_bridge_same(dp, other_dp))
> > continue;
... this condition will filter all cases where it is NULL. For
dsa_port_bridge_same() to return true both ports need to be attached
to a bridge (and to the same bridge), and to be attached to a bridge a
net_device is required, so other_dp->user cannot be NULL. And we only
access user after here.
Best regards,
Jonas
On Mon, 1 Dec 2025 20:52:34 +0100 Jonas Gorski <jonas.gorski@gmail.com> wrote:
> Hi,
>
> On Mon, Dec 1, 2025 at 3:48 PM Simon Horman <horms@kernel.org> wrote:
> >
> > On Mon, Dec 01, 2025 at 11:28:13AM +0100, Jonas Gorski wrote:
> >
> > ...
> >
> > > diff --git a/net/dsa/user.c b/net/dsa/user.c
> > > index f59d66f0975d..fa1fe0f1493a 100644
> > > --- a/net/dsa/user.c
> > > +++ b/net/dsa/user.c
> > > @@ -653,21 +653,30 @@ static int dsa_user_port_attr_set(struct net_device *dev, const void *ctx,
> > >
> > > /* Must be called under rcu_read_lock() */
> > > static int
> > > -dsa_user_vlan_check_for_8021q_uppers(struct net_device *user,
> > > +dsa_user_vlan_check_for_8021q_uppers(struct dsa_port *dp,
> > > const struct switchdev_obj_port_vlan *vlan)
> > > {
> > > - struct net_device *upper_dev;
> > > - struct list_head *iter;
> > > + struct dsa_switch *ds = dp->ds;
> > > + struct dsa_port *other_dp;
> > >
> > > - netdev_for_each_upper_dev_rcu(user, upper_dev, iter) {
> > > - u16 vid;
> > > + dsa_switch_for_each_user_port(other_dp, ds) {
> > > + struct net_device *user = other_dp->user;
> >
> > Hi Jonas,
> >
> > The AI robot is concerned that user may be NULL here.
> > And I can't convince myself that cannot be the case.
> >
> > Could you take a look?
> >
> > https://netdev-ai.bots.linux.dev/ai-review.html?id=47057e-e740-4b66-9d60-9ec2a7ee92a1#patch-0
>
> At this point it can be NULL. But it being NULL is not an issue, as ...
> >
> > > + struct net_device *upper_dev;
> > > + struct list_head *iter;
> > >
> > > - if (!is_vlan_dev(upper_dev))
> > > + if (!dsa_port_bridge_same(dp, other_dp))
> > > continue;
>
> ... this condition will filter all cases where it is NULL. For
> dsa_port_bridge_same() to return true both ports need to be attached
> to a bridge (and to the same bridge), and to be attached to a bridge a
> net_device is required, so other_dp->user cannot be NULL. And we only
> access user after here.
I reproduced this false positive here, thanks for the explanation. This is an
example of a class of review mistakes I've wanted to fix, so I used it to
improve the prompts around NULL pointers that are protected via other checks.
I'll test this on some more commits and push it out.
-chris
On Mon, Dec 01, 2025 at 02:48:48PM -0800, Chris Mason wrote:
> On Mon, 1 Dec 2025 20:52:34 +0100 Jonas Gorski <jonas.gorski@gmail.com> wrote:
>
> > Hi,
> >
> > On Mon, Dec 1, 2025 at 3:48 PM Simon Horman <horms@kernel.org> wrote:
> > >
> > > On Mon, Dec 01, 2025 at 11:28:13AM +0100, Jonas Gorski wrote:
> > >
> > > ...
> > >
> > > > diff --git a/net/dsa/user.c b/net/dsa/user.c
> > > > index f59d66f0975d..fa1fe0f1493a 100644
> > > > --- a/net/dsa/user.c
> > > > +++ b/net/dsa/user.c
> > > > @@ -653,21 +653,30 @@ static int dsa_user_port_attr_set(struct net_device *dev, const void *ctx,
> > > >
> > > > /* Must be called under rcu_read_lock() */
> > > > static int
> > > > -dsa_user_vlan_check_for_8021q_uppers(struct net_device *user,
> > > > +dsa_user_vlan_check_for_8021q_uppers(struct dsa_port *dp,
> > > > const struct switchdev_obj_port_vlan *vlan)
> > > > {
> > > > - struct net_device *upper_dev;
> > > > - struct list_head *iter;
> > > > + struct dsa_switch *ds = dp->ds;
> > > > + struct dsa_port *other_dp;
> > > >
> > > > - netdev_for_each_upper_dev_rcu(user, upper_dev, iter) {
> > > > - u16 vid;
> > > > + dsa_switch_for_each_user_port(other_dp, ds) {
> > > > + struct net_device *user = other_dp->user;
> > >
> > > Hi Jonas,
> > >
> > > The AI robot is concerned that user may be NULL here.
> > > And I can't convince myself that cannot be the case.
> > >
> > > Could you take a look?
> > >
> > > https://netdev-ai.bots.linux.dev/ai-review.html?id=47057e-e740-4b66-9d60-9ec2a7ee92a1#patch-0
> >
> > At this point it can be NULL. But it being NULL is not an issue, as ...
> > >
> > > > + struct net_device *upper_dev;
> > > > + struct list_head *iter;
> > > >
> > > > - if (!is_vlan_dev(upper_dev))
> > > > + if (!dsa_port_bridge_same(dp, other_dp))
> > > > continue;
> >
> > ... this condition will filter all cases where it is NULL. For
> > dsa_port_bridge_same() to return true both ports need to be attached
> > to a bridge (and to the same bridge), and to be attached to a bridge a
> > net_device is required, so other_dp->user cannot be NULL. And we only
> > access user after here.
Thanks for the explanation Jonas.
I wasn't very confident with this report.
And I was too focused on working out if user could be NULL rather
than if it matters. Still, I may not have worked it out.
>
> I reproduced this false positive here, thanks for the explanation. This is an
> example of a class of review mistakes I've wanted to fix, so I used it to
> improve the prompts around NULL pointers that are protected via other checks.
>
> I'll test this on some more commits and push it out.
Thanks for following-up on this Chris.
I guess everyone has their own opinion on AI.
And, in a similar vein, many have opinions on the review-prompts.
But, FTR, I've been impressed by the output I've seen,
having used them for a few weeks now. And I look forward
to that improving further.
On 12/2/25 11:16 AM, Simon Horman wrote: > On Mon, Dec 01, 2025 at 02:48:48PM -0800, Chris Mason wrote: >> On Mon, 1 Dec 2025 20:52:34 +0100 Jonas Gorski <jonas.gorski@gmail.com> wrote: [ ... ] >> >> I reproduced this false positive here, thanks for the explanation. This is an >> example of a class of review mistakes I've wanted to fix, so I used it to >> improve the prompts around NULL pointers that are protected via other checks. >> >> I'll test this on some more commits and push it out. > > Thanks for following-up on this Chris. > > I guess everyone has their own opinion on AI. > And, in a similar vein, many have opinions on the review-prompts. > But, FTR, I've been impressed by the output I've seen, > having used them for a few weeks now. And I look forward > to that improving further. > Thanks, I really appreciate everyone being willing to experiment with the reviews (and huge thanks to Jakub for wiring this up). I'll try to stay on top of the false positives, but please also let me know if I'm missing classes of bugs. I think Jonas's other_dp->user check is really at the limit of claude's ability to reason about the code in a generic way, and I'm still trying to get it to process this patch consistently. If all else fails we can fix some of these with more networking specific knowledge, but I'm trying to avoid whack-a-mole. -chris
© 2016 - 2026 Red Hat, Inc.