| Message ID | 20170515110819.11847-1-tobias.jungel@bisdn.de |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
Hi Tobias, 2017-05-15, 13:08:19 +0200, Tobias Jungel wrote: > Currently it is allowed to set the default pvid of a bridge to a value > above VLAN_VID_MASK (0xfff). This patch checks the passed pvid and > disables the pvid in case it is out of bounds. Could we return an error (-EINVAL) to userspace instead? Silently disabling the feature seems confusing to me. This would probably be better in br_validate() (like the IFLA_BR_VLAN_PROTOCOL check), since there's already such a check when setting default_pvid from sysfs (in br_vlan_set_default_pvid()). > > Reproduce by calling: > > [root@test ~]# ip l a type bridge > [root@test ~]# ip l a type dummy > [root@test ~]# ip l s bridge0 type bridge vlan_filtering 1 > [root@test ~]# ip l s bridge0 type bridge vlan_default_pvid 9999 > [root@test ~]# ip l s dummy0 master bridge0 > [root@test ~]# bridge vlan > port vlan ids > bridge0 9999 PVID Egress Untagged > > dummy0 9999 PVID Egress Untagged You'll also need to add a Signed-off-by, and a Fixes tag would be nice. Thanks,
On 5/15/17 2:08 PM, Tobias Jungel wrote: > Currently it is allowed to set the default pvid of a bridge to a value > above VLAN_VID_MASK (0xfff). This patch checks the passed pvid and > disables the pvid in case it is out of bounds. > > Reproduce by calling: > > [root@test ~]# ip l a type bridge > [root@test ~]# ip l a type dummy > [root@test ~]# ip l s bridge0 type bridge vlan_filtering 1 > [root@test ~]# ip l s bridge0 type bridge vlan_default_pvid 9999 > [root@test ~]# ip l s dummy0 master bridge0 > [root@test ~]# bridge vlan > port vlan ids > bridge0 9999 PVID Egress Untagged > > dummy0 9999 PVID Egress Untagged > --- > net/bridge/br_vlan.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > Good catch, but this is not the right fix. Default pvid of 0 disables the default pvid, but a wrong pvid should result in -EINVAL return instead of 0. Take a look how the parent default pvid function handles this case (br_vlan_set_default_pvid). Also please add a sign off and a proper Fixes tag. For more information about submitting patches you can check Documentation/SubmittingPatches Thanks, Nik
Thanks Sabrina and Nik. On Mon, 2017-05-15 at 14:01 +0200, Sabrina Dubroca wrote: > Hi Tobias, > > 2017-05-15, 13:08:19 +0200, Tobias Jungel wrote: > > Currently it is allowed to set the default pvid of a bridge to a > > value > > above VLAN_VID_MASK (0xfff). This patch checks the passed pvid and > > disables the pvid in case it is out of bounds. > > Could we return an error (-EINVAL) to userspace instead? Silently > disabling the feature seems confusing to me. This would probably be > better in br_validate() (like the IFLA_BR_VLAN_PROTOCOL check), since > there's already such a check when setting default_pvid from sysfs (in > br_vlan_set_default_pvid()). I will send a v2 that returns -EINVAL. br_validate seems to be the wrong place to me since it deals with the bridge ports. > > > > > Reproduce by calling: > > > > [root@test ~]# ip l a type bridge > > [root@test ~]# ip l a type dummy > > [root@test ~]# ip l s bridge0 type bridge vlan_filtering 1 > > [root@test ~]# ip l s bridge0 type bridge vlan_default_pvid 9999 > > [root@test ~]# ip l s dummy0 master bridge0 > > [root@test ~]# bridge vlan > > port vlan ids > > bridge0 9999 PVID Egress Untagged > > > > dummy0 9999 PVID Egress Untagged > > You'll also need to add a Signed-off-by, and a Fixes tag would be > nice. > Right, will add this as well. > > Thanks, >
On 5/15/17 4:21 PM, Tobias Jungel wrote: > Thanks Sabrina and Nik. > > On Mon, 2017-05-15 at 14:01 +0200, Sabrina Dubroca wrote: >> Hi Tobias, >> >> 2017-05-15, 13:08:19 +0200, Tobias Jungel wrote: >>> Currently it is allowed to set the default pvid of a bridge to a >>> value >>> above VLAN_VID_MASK (0xfff). This patch checks the passed pvid and >>> disables the pvid in case it is out of bounds. >> >> Could we return an error (-EINVAL) to userspace instead? Silently >> disabling the feature seems confusing to me. This would probably be >> better in br_validate() (like the IFLA_BR_VLAN_PROTOCOL check), since >> there's already such a check when setting default_pvid from sysfs (in >> br_vlan_set_default_pvid()). > > I will send a v2 that returns -EINVAL. br_validate seems to be the > wrong place to me since it deals with the bridge ports. > Could you elaborate ? br_validate should be called for all and is a very good suggestion. >> >>> >>> Reproduce by calling: >>> >>> [root@test ~]# ip l a type bridge >>> [root@test ~]# ip l a type dummy >>> [root@test ~]# ip l s bridge0 type bridge vlan_filtering 1 >>> [root@test ~]# ip l s bridge0 type bridge vlan_default_pvid 9999 >>> [root@test ~]# ip l s dummy0 master bridge0 >>> [root@test ~]# bridge vlan >>> port vlan ids >>> bridge0 9999 PVID Egress Untagged >>> >>> dummy0 9999 PVID Egress Untagged >> >> You'll also need to add a Signed-off-by, and a Fixes tag would be >> nice. >> > > Right, will add this as well. > >> >> Thanks, >>
On 5/15/17 4:29 PM, Nikolay Aleksandrov wrote: > On 5/15/17 4:21 PM, Tobias Jungel wrote: >> Thanks Sabrina and Nik. >> >> On Mon, 2017-05-15 at 14:01 +0200, Sabrina Dubroca wrote: >>> Hi Tobias, >>> >>> 2017-05-15, 13:08:19 +0200, Tobias Jungel wrote: >>>> Currently it is allowed to set the default pvid of a bridge to a >>>> value >>>> above VLAN_VID_MASK (0xfff). This patch checks the passed pvid and >>>> disables the pvid in case it is out of bounds. >>> >>> Could we return an error (-EINVAL) to userspace instead? Silently >>> disabling the feature seems confusing to me. This would probably be >>> better in br_validate() (like the IFLA_BR_VLAN_PROTOCOL check), since >>> there's already such a check when setting default_pvid from sysfs (in >>> br_vlan_set_default_pvid()). >> >> I will send a v2 that returns -EINVAL. br_validate seems to be the >> wrong place to me since it deals with the bridge ports. >> > > Could you elaborate ? br_validate should be called for all and is a very good > suggestion. I meant for the bridge newlink/changelink of course. :-) > >>> >>>> >>>> Reproduce by calling: >>>> >>>> [root@test ~]# ip l a type bridge >>>> [root@test ~]# ip l a type dummy >>>> [root@test ~]# ip l s bridge0 type bridge vlan_filtering 1 >>>> [root@test ~]# ip l s bridge0 type bridge vlan_default_pvid 9999 >>>> [root@test ~]# ip l s dummy0 master bridge0 >>>> [root@test ~]# bridge vlan >>>> port vlan ids >>>> bridge0 9999 PVID Egress Untagged >>>> >>>> dummy0 9999 PVID Egress Untagged >>> >>> You'll also need to add a Signed-off-by, and a Fixes tag would be >>> nice. >>> >> >> Right, will add this as well. >> >>> >>> Thanks, >>> >
On Mon, 2017-05-15 at 16:31 +0300, Nikolay Aleksandrov wrote: > On 5/15/17 4:29 PM, Nikolay Aleksandrov wrote: > > On 5/15/17 4:21 PM, Tobias Jungel wrote: > > > Thanks Sabrina and Nik. > > > > > > On Mon, 2017-05-15 at 14:01 +0200, Sabrina Dubroca wrote: > > > > Hi Tobias, > > > > > > > > 2017-05-15, 13:08:19 +0200, Tobias Jungel wrote: > > > > > Currently it is allowed to set the default pvid of a bridge > > > > > to a > > > > > value > > > > > above VLAN_VID_MASK (0xfff). This patch checks the passed > > > > > pvid and > > > > > disables the pvid in case it is out of bounds. > > > > > > > > Could we return an error (-EINVAL) to userspace > > > > instead? Silently > > > > disabling the feature seems confusing to me. This would > > > > probably be > > > > better in br_validate() (like the IFLA_BR_VLAN_PROTOCOL check), > > > > since > > > > there's already such a check when setting default_pvid from > > > > sysfs (in > > > > br_vlan_set_default_pvid()). > > > > > > I will send a v2 that returns -EINVAL. br_validate seems to be > > > the > > > wrong place to me since it deals with the bridge ports. > > > > > > > Could you elaborate ? br_validate should be called for all and is a > > very good > > suggestion. > > I meant for the bridge newlink/changelink of course. :-) Sorry had a wrong understanding of that function. Will come up with a v3 later. > > > > > > > > > > > > > > > > > Reproduce by calling: > > > > > > > > > > [root@test ~]# ip l a type bridge > > > > > [root@test ~]# ip l a type dummy > > > > > [root@test ~]# ip l s bridge0 type bridge vlan_filtering 1 > > > > > [root@test ~]# ip l s bridge0 type bridge vlan_default_pvid > > > > > 9999 > > > > > [root@test ~]# ip l s dummy0 master bridge0 > > > > > [root@test ~]# bridge vlan > > > > > port vlan ids > > > > > bridge0 9999 PVID Egress Untagged > > > > > > > > > > dummy0 9999 PVID Egress Untagged > > > > > > > > You'll also need to add a Signed-off-by, and a Fixes tag would > > > > be > > > > nice. > > > > > > > > > > Right, will add this as well. > > > > > > > > > > > Thanks, > > > > > >
diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c index b838213..e363d75 100644 --- a/net/bridge/br_vlan.c +++ b/net/bridge/br_vlan.c @@ -820,7 +820,7 @@ int __br_vlan_set_default_pvid(struct net_bridge *br, u16 pvid) int err = 0; unsigned long *changed; - if (!pvid) { + if (!pvid || pvid >= VLAN_VID_MASK) { br_vlan_disable_default_pvid(br); return 0; }