Re: [PATCH net-next] switchdev: enforce no pvid flag in vlan ranges
Thu, Oct 15, 2015 at 05:58:33AM IDT, sfel...@gmail.com wrote:
>On Wed, Oct 14, 2015 at 10:42 AM, Ido Schimmel wrote:
>> Wed, Oct 14, 2015 at 08:14:24PM IDT, sfel...@gmail.com wrote:
>>>On Wed, Oct 14, 2015 at 8:25 AM, Vivien Didelot
>>> wrote:
On Oct. Wednesday 14 (42) 09:14 AM, Ido Schimmel wrote:
> Tue, Oct 13, 2015 at 05:32:26PM IDT, vivien.dide...@savoirfairelinux.com
> wrote:
> >On Oct. Tuesday 13 (42) 11:31 AM, Ido Schimmel wrote:
> >> Mon, Oct 12, 2015 at 08:36:25PM IDT,
> >> vivien.dide...@savoirfairelinux.com wrote:
> >> >Hi guys,
> >> >
> >> >On Oct. Monday 12 (42) 02:01 PM, Nikolay Aleksandrov wrote:
> >> >> From: Nikolay Aleksandrov
> >> >>
> >> >> We shouldn't allow BRIDGE_VLAN_INFO_PVID flag in VLAN ranges.
> >> >>
> >> >> Signed-off-by: Nikolay Aleksandrov
> >> >> ---
> >> >> net/switchdev/switchdev.c | 3 +++
> >> >> 1 file changed, 3 insertions(+)
> >> >>
> >> >> diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
> >> >> index 6e4a4f9ad927..256c596de896 100644
> >> >> --- a/net/switchdev/switchdev.c
> >> >> +++ b/net/switchdev/switchdev.c
> >> >> @@ -720,6 +720,9 @@ static int switchdev_port_br_afspec(struct
> >> >> net_device *dev,
> >> >> if (vlan.vid_begin)
> >> >> return -EINVAL;
> >> >> vlan.vid_begin = vinfo->vid;
> >> >> + /* don't allow range of pvids */
> >> >> + if (vlan.flags & BRIDGE_VLAN_INFO_PVID)
> >> >> + return -EINVAL;
> >> >> } else if (vinfo->flags &
> >> >> BRIDGE_VLAN_INFO_RANGE_END) {
> >> >> if (!vlan.vid_begin)
> >> >> return -EINVAL;
> >> >> --
> >> >> 2.4.3
> >> >>
> >> >
> >> >Yes the patch looks good, but it is a minor check though. I hope the
> >> >subject of this thread is making sense.
> >> >
> >> >VLAN ranges seem to have been included for an UX purpose (so commands
> >> >look like Cisco IOS). We don't want to change any existing interface,
> >> >so
> >> >we pushed that down to drivers, with the only valid reason that, maybe
> >> >one day, an hardware can be capable of programming a range on a
> >> >per-port
> >> >basis.
> >> Hi,
> >>
> >> That's actually what we are doing in mlxsw. We can do up to 256
> >> entries in
> >> one go. We've yet to submit this part.
> >
> >Perfect Ido, thanks for pointing this out! I'm OK with the range then.
> >
> >So there is now a very last question in my head for this, which is more
> >a matter of kernel design. Should the user be aware of such underlying
> >support? In other words, would it make sense to do this in a driver:
> >
> >foo_port_vlan_add(struct net_device *dev,
> > struct switchdev_obj_port_vlan *vlan)
> >{
> >if (vlan->vid_begin != vlan->vid_end)
> >return -ENOTSUPP; /* or something more relevant for user */
> >
> >return foo_port_single_vlan_add(dev, vlan->vid_begin);
> >}
> >
> >So drivers keep being simple, and we can easily propagate the fact that
> >one-or-all VLAN is not supportable, vs. the VLAN feature itself is not
> >implemented and must be done in software.
> I think that if you want to keep it simple, then Scott's advice from the
> previous thread is the most appropriate one. I believe the hardware you
> are using is simply not meant to support multiple 802.1Q bridges.
You mean allowing only one Linux bridge over an hardware switch?
It would for sure simplify how, as developers and users, we represent a
physical switch. But I am not sure how to achieve that and I don't have
strong opinions on this TBH.
>>>
>>>Hi Vivien, I think it's possible to keep switch ports on just one
>>>bridge if we do a little bit of work on the NETDEV_CHANGEUPPER
>>>notifier. This will give you the driver-level control you want. Do
>>>you have time to investigate? The idea is:
>>>
>>>1) In your driver's handler for NETDEV_CHANGEUPPER, if switch port is
>>>being added to a second bridge,then return NOTIFY_BAD. Your driver
>>>needs to track the bridge count.
>>>
>>>2) In __netdev_upper_dev_link(), check the return code from the
>>>call_netdevice_notifiers_info(NETDEV_CHANGEUPPER, ...) call, and if
>>>NOTIFY_BAD, abort the linking operation (goto rollback_xxx).
>>>
>> Hi,
>>
>> We are doing something similar in mlxsw (not upstream yet). Jiri
>> introduced PRE_CHANGEUPPER, which is called from the function you
>> mentioned, but before the linking operation (so that you don't need to
>> rollback).
>
>Oh, cool.
>
>> If the notification is about a linking operation and the master is a
>> bridge
Re: [PATCH net-next] switchdev: enforce no pvid flag in vlan ranges
On Wed, Oct 14, 2015 at 10:42 AM, Ido Schimmel wrote:
> Wed, Oct 14, 2015 at 08:14:24PM IDT, sfel...@gmail.com wrote:
>>On Wed, Oct 14, 2015 at 8:25 AM, Vivien Didelot
>> wrote:
>>> On Oct. Wednesday 14 (42) 09:14 AM, Ido Schimmel wrote:
Tue, Oct 13, 2015 at 05:32:26PM IDT, vivien.dide...@savoirfairelinux.com
wrote:
>On Oct. Tuesday 13 (42) 11:31 AM, Ido Schimmel wrote:
>> Mon, Oct 12, 2015 at 08:36:25PM IDT,
>> vivien.dide...@savoirfairelinux.com wrote:
>> >Hi guys,
>> >
>> >On Oct. Monday 12 (42) 02:01 PM, Nikolay Aleksandrov wrote:
>> >> From: Nikolay Aleksandrov
>> >>
>> >> We shouldn't allow BRIDGE_VLAN_INFO_PVID flag in VLAN ranges.
>> >>
>> >> Signed-off-by: Nikolay Aleksandrov
>> >> ---
>> >> net/switchdev/switchdev.c | 3 +++
>> >> 1 file changed, 3 insertions(+)
>> >>
>> >> diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
>> >> index 6e4a4f9ad927..256c596de896 100644
>> >> --- a/net/switchdev/switchdev.c
>> >> +++ b/net/switchdev/switchdev.c
>> >> @@ -720,6 +720,9 @@ static int switchdev_port_br_afspec(struct
>> >> net_device *dev,
>> >> if (vlan.vid_begin)
>> >> return -EINVAL;
>> >> vlan.vid_begin = vinfo->vid;
>> >> + /* don't allow range of pvids */
>> >> + if (vlan.flags & BRIDGE_VLAN_INFO_PVID)
>> >> + return -EINVAL;
>> >> } else if (vinfo->flags &
>> >> BRIDGE_VLAN_INFO_RANGE_END) {
>> >> if (!vlan.vid_begin)
>> >> return -EINVAL;
>> >> --
>> >> 2.4.3
>> >>
>> >
>> >Yes the patch looks good, but it is a minor check though. I hope the
>> >subject of this thread is making sense.
>> >
>> >VLAN ranges seem to have been included for an UX purpose (so commands
>> >look like Cisco IOS). We don't want to change any existing interface,
>> >so
>> >we pushed that down to drivers, with the only valid reason that, maybe
>> >one day, an hardware can be capable of programming a range on a
>> >per-port
>> >basis.
>> Hi,
>>
>> That's actually what we are doing in mlxsw. We can do up to 256 entries
>> in
>> one go. We've yet to submit this part.
>
>Perfect Ido, thanks for pointing this out! I'm OK with the range then.
>
>So there is now a very last question in my head for this, which is more
>a matter of kernel design. Should the user be aware of such underlying
>support? In other words, would it make sense to do this in a driver:
>
>foo_port_vlan_add(struct net_device *dev,
> struct switchdev_obj_port_vlan *vlan)
>{
>if (vlan->vid_begin != vlan->vid_end)
>return -ENOTSUPP; /* or something more relevant for user */
>
>return foo_port_single_vlan_add(dev, vlan->vid_begin);
>}
>
>So drivers keep being simple, and we can easily propagate the fact that
>one-or-all VLAN is not supportable, vs. the VLAN feature itself is not
>implemented and must be done in software.
I think that if you want to keep it simple, then Scott's advice from the
previous thread is the most appropriate one. I believe the hardware you
are using is simply not meant to support multiple 802.1Q bridges.
>>>
>>> You mean allowing only one Linux bridge over an hardware switch?
>>>
>>> It would for sure simplify how, as developers and users, we represent a
>>> physical switch. But I am not sure how to achieve that and I don't have
>>> strong opinions on this TBH.
>>
>>Hi Vivien, I think it's possible to keep switch ports on just one
>>bridge if we do a little bit of work on the NETDEV_CHANGEUPPER
>>notifier. This will give you the driver-level control you want. Do
>>you have time to investigate? The idea is:
>>
>>1) In your driver's handler for NETDEV_CHANGEUPPER, if switch port is
>>being added to a second bridge,then return NOTIFY_BAD. Your driver
>>needs to track the bridge count.
>>
>>2) In __netdev_upper_dev_link(), check the return code from the
>>call_netdevice_notifiers_info(NETDEV_CHANGEUPPER, ...) call, and if
>>NOTIFY_BAD, abort the linking operation (goto rollback_xxx).
>>
> Hi,
>
> We are doing something similar in mlxsw (not upstream yet). Jiri
> introduced PRE_CHANGEUPPER, which is called from the function you
> mentioned, but before the linking operation (so that you don't need to
> rollback).
Oh, cool.
> If the notification is about a linking operation and the master is a
> bridge different than the current one, then NOTIFY_BAD is returned.
So you're wanting to restrict to just one bridge also? Or is
NOTIFY_BAD returned for some other reason? I g
Re: [PATCH net-next] switchdev: enforce no pvid flag in vlan ranges
On Oct. Wednesday 14 (42) 03:08 PM, Florian Fainelli wrote:
> On 14/10/15 11:51, Vivien Didelot wrote:
> > On Oct. Wednesday 14 (42) 08:42 PM, Ido Schimmel wrote:
> >> Wed, Oct 14, 2015 at 08:14:24PM IDT, sfel...@gmail.com wrote:
> >>> On Wed, Oct 14, 2015 at 8:25 AM, Vivien Didelot
> >>> wrote:
> On Oct. Wednesday 14 (42) 09:14 AM, Ido Schimmel wrote:
> > Tue, Oct 13, 2015 at 05:32:26PM IDT,
> > vivien.dide...@savoirfairelinux.com wrote:
> >> On Oct. Tuesday 13 (42) 11:31 AM, Ido Schimmel wrote:
> >>> Mon, Oct 12, 2015 at 08:36:25PM IDT,
> >>> vivien.dide...@savoirfairelinux.com wrote:
> Hi guys,
>
> On Oct. Monday 12 (42) 02:01 PM, Nikolay Aleksandrov wrote:
> > From: Nikolay Aleksandrov
> >
> > We shouldn't allow BRIDGE_VLAN_INFO_PVID flag in VLAN ranges.
> >
> > Signed-off-by: Nikolay Aleksandrov
> > ---
> > net/switchdev/switchdev.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
> > index 6e4a4f9ad927..256c596de896 100644
> > --- a/net/switchdev/switchdev.c
> > +++ b/net/switchdev/switchdev.c
> > @@ -720,6 +720,9 @@ static int switchdev_port_br_afspec(struct
> > net_device *dev,
> > if (vlan.vid_begin)
> > return -EINVAL;
> > vlan.vid_begin = vinfo->vid;
> > + /* don't allow range of pvids */
> > + if (vlan.flags & BRIDGE_VLAN_INFO_PVID)
> > + return -EINVAL;
> > } else if (vinfo->flags &
> > BRIDGE_VLAN_INFO_RANGE_END) {
> > if (!vlan.vid_begin)
> > return -EINVAL;
> > --
> > 2.4.3
> >
>
> Yes the patch looks good, but it is a minor check though. I hope the
> subject of this thread is making sense.
>
> VLAN ranges seem to have been included for an UX purpose (so commands
> look like Cisco IOS). We don't want to change any existing
> interface, so
> we pushed that down to drivers, with the only valid reason that,
> maybe
> one day, an hardware can be capable of programming a range on a
> per-port
> basis.
> >>> Hi,
> >>>
> >>> That's actually what we are doing in mlxsw. We can do up to 256
> >>> entries in
> >>> one go. We've yet to submit this part.
> >>
> >> Perfect Ido, thanks for pointing this out! I'm OK with the range then.
> >>
> >> So there is now a very last question in my head for this, which is more
> >> a matter of kernel design. Should the user be aware of such underlying
> >> support? In other words, would it make sense to do this in a driver:
> >>
> >>foo_port_vlan_add(struct net_device *dev,
> >> struct switchdev_obj_port_vlan *vlan)
> >>{
> >>if (vlan->vid_begin != vlan->vid_end)
> >>return -ENOTSUPP; /* or something more relevant for user */
> >>
> >>return foo_port_single_vlan_add(dev, vlan->vid_begin);
> >>}
> >>
> >> So drivers keep being simple, and we can easily propagate the fact that
> >> one-or-all VLAN is not supportable, vs. the VLAN feature itself is not
> >> implemented and must be done in software.
> > I think that if you want to keep it simple, then Scott's advice from the
> > previous thread is the most appropriate one. I believe the hardware you
> > are using is simply not meant to support multiple 802.1Q bridges.
>
> You mean allowing only one Linux bridge over an hardware switch?
>
> It would for sure simplify how, as developers and users, we represent a
> physical switch. But I am not sure how to achieve that and I don't have
> strong opinions on this TBH.
> >>>
> >>> Hi Vivien, I think it's possible to keep switch ports on just one
> >>> bridge if we do a little bit of work on the NETDEV_CHANGEUPPER
> >>> notifier. This will give you the driver-level control you want. Do
> >>> you have time to investigate? The idea is:
> >>>
> >>> 1) In your driver's handler for NETDEV_CHANGEUPPER, if switch port is
> >>> being added to a second bridge,then return NOTIFY_BAD. Your driver
> >>> needs to track the bridge count.
> >>>
> >>> 2) In __netdev_upper_dev_link(), check the return code from the
> >>> call_netdevice_notifiers_info(NETDEV_CHANGEUPPER, ...) call, and if
> >>> NOTIFY_BAD, abort the linking operation (goto rollback_xxx).
> >>>
> >> Hi,
> >>
> >> We are doing something similar in mlxsw (not upstream yet). Jiri
> >> introduced PRE_CHANGEUPPER, which is called from the function you
> >
Re: [PATCH net-next] switchdev: enforce no pvid flag in vlan ranges
On 14/10/15 11:51, Vivien Didelot wrote:
> On Oct. Wednesday 14 (42) 08:42 PM, Ido Schimmel wrote:
>> Wed, Oct 14, 2015 at 08:14:24PM IDT, sfel...@gmail.com wrote:
>>> On Wed, Oct 14, 2015 at 8:25 AM, Vivien Didelot
>>> wrote:
On Oct. Wednesday 14 (42) 09:14 AM, Ido Schimmel wrote:
> Tue, Oct 13, 2015 at 05:32:26PM IDT, vivien.dide...@savoirfairelinux.com
> wrote:
>> On Oct. Tuesday 13 (42) 11:31 AM, Ido Schimmel wrote:
>>> Mon, Oct 12, 2015 at 08:36:25PM IDT,
>>> vivien.dide...@savoirfairelinux.com wrote:
Hi guys,
On Oct. Monday 12 (42) 02:01 PM, Nikolay Aleksandrov wrote:
> From: Nikolay Aleksandrov
>
> We shouldn't allow BRIDGE_VLAN_INFO_PVID flag in VLAN ranges.
>
> Signed-off-by: Nikolay Aleksandrov
> ---
> net/switchdev/switchdev.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
> index 6e4a4f9ad927..256c596de896 100644
> --- a/net/switchdev/switchdev.c
> +++ b/net/switchdev/switchdev.c
> @@ -720,6 +720,9 @@ static int switchdev_port_br_afspec(struct
> net_device *dev,
> if (vlan.vid_begin)
> return -EINVAL;
> vlan.vid_begin = vinfo->vid;
> + /* don't allow range of pvids */
> + if (vlan.flags & BRIDGE_VLAN_INFO_PVID)
> + return -EINVAL;
> } else if (vinfo->flags & BRIDGE_VLAN_INFO_RANGE_END)
> {
> if (!vlan.vid_begin)
> return -EINVAL;
> --
> 2.4.3
>
Yes the patch looks good, but it is a minor check though. I hope the
subject of this thread is making sense.
VLAN ranges seem to have been included for an UX purpose (so commands
look like Cisco IOS). We don't want to change any existing interface,
so
we pushed that down to drivers, with the only valid reason that, maybe
one day, an hardware can be capable of programming a range on a
per-port
basis.
>>> Hi,
>>>
>>> That's actually what we are doing in mlxsw. We can do up to 256 entries
>>> in
>>> one go. We've yet to submit this part.
>>
>> Perfect Ido, thanks for pointing this out! I'm OK with the range then.
>>
>> So there is now a very last question in my head for this, which is more
>> a matter of kernel design. Should the user be aware of such underlying
>> support? In other words, would it make sense to do this in a driver:
>>
>>foo_port_vlan_add(struct net_device *dev,
>> struct switchdev_obj_port_vlan *vlan)
>>{
>>if (vlan->vid_begin != vlan->vid_end)
>>return -ENOTSUPP; /* or something more relevant for user */
>>
>>return foo_port_single_vlan_add(dev, vlan->vid_begin);
>>}
>>
>> So drivers keep being simple, and we can easily propagate the fact that
>> one-or-all VLAN is not supportable, vs. the VLAN feature itself is not
>> implemented and must be done in software.
> I think that if you want to keep it simple, then Scott's advice from the
> previous thread is the most appropriate one. I believe the hardware you
> are using is simply not meant to support multiple 802.1Q bridges.
You mean allowing only one Linux bridge over an hardware switch?
It would for sure simplify how, as developers and users, we represent a
physical switch. But I am not sure how to achieve that and I don't have
strong opinions on this TBH.
>>>
>>> Hi Vivien, I think it's possible to keep switch ports on just one
>>> bridge if we do a little bit of work on the NETDEV_CHANGEUPPER
>>> notifier. This will give you the driver-level control you want. Do
>>> you have time to investigate? The idea is:
>>>
>>> 1) In your driver's handler for NETDEV_CHANGEUPPER, if switch port is
>>> being added to a second bridge,then return NOTIFY_BAD. Your driver
>>> needs to track the bridge count.
>>>
>>> 2) In __netdev_upper_dev_link(), check the return code from the
>>> call_netdevice_notifiers_info(NETDEV_CHANGEUPPER, ...) call, and if
>>> NOTIFY_BAD, abort the linking operation (goto rollback_xxx).
>>>
>> Hi,
>>
>> We are doing something similar in mlxsw (not upstream yet). Jiri
>> introduced PRE_CHANGEUPPER, which is called from the function you
>> mentioned, but before the linking operation (so that you don't need to
>> rollback).
>>
>> If the notification is about a linking operation and the master is a
>> bridge different than the current one, then NOTIFY_BAD is returned.
>
> Great, I'll wait for this then.
>
> Scott,
Re: [PATCH net-next] switchdev: enforce no pvid flag in vlan ranges
On Oct. Wednesday 14 (42) 08:42 PM, Ido Schimmel wrote:
> Wed, Oct 14, 2015 at 08:14:24PM IDT, sfel...@gmail.com wrote:
> >On Wed, Oct 14, 2015 at 8:25 AM, Vivien Didelot
> > wrote:
> >> On Oct. Wednesday 14 (42) 09:14 AM, Ido Schimmel wrote:
> >>> Tue, Oct 13, 2015 at 05:32:26PM IDT, vivien.dide...@savoirfairelinux.com
> >>> wrote:
> >>> >On Oct. Tuesday 13 (42) 11:31 AM, Ido Schimmel wrote:
> >>> >> Mon, Oct 12, 2015 at 08:36:25PM IDT,
> >>> >> vivien.dide...@savoirfairelinux.com wrote:
> >>> >> >Hi guys,
> >>> >> >
> >>> >> >On Oct. Monday 12 (42) 02:01 PM, Nikolay Aleksandrov wrote:
> >>> >> >> From: Nikolay Aleksandrov
> >>> >> >>
> >>> >> >> We shouldn't allow BRIDGE_VLAN_INFO_PVID flag in VLAN ranges.
> >>> >> >>
> >>> >> >> Signed-off-by: Nikolay Aleksandrov
> >>> >> >> ---
> >>> >> >> net/switchdev/switchdev.c | 3 +++
> >>> >> >> 1 file changed, 3 insertions(+)
> >>> >> >>
> >>> >> >> diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
> >>> >> >> index 6e4a4f9ad927..256c596de896 100644
> >>> >> >> --- a/net/switchdev/switchdev.c
> >>> >> >> +++ b/net/switchdev/switchdev.c
> >>> >> >> @@ -720,6 +720,9 @@ static int switchdev_port_br_afspec(struct
> >>> >> >> net_device *dev,
> >>> >> >> if (vlan.vid_begin)
> >>> >> >> return -EINVAL;
> >>> >> >> vlan.vid_begin = vinfo->vid;
> >>> >> >> + /* don't allow range of pvids */
> >>> >> >> + if (vlan.flags & BRIDGE_VLAN_INFO_PVID)
> >>> >> >> + return -EINVAL;
> >>> >> >> } else if (vinfo->flags &
> >>> >> >> BRIDGE_VLAN_INFO_RANGE_END) {
> >>> >> >> if (!vlan.vid_begin)
> >>> >> >> return -EINVAL;
> >>> >> >> --
> >>> >> >> 2.4.3
> >>> >> >>
> >>> >> >
> >>> >> >Yes the patch looks good, but it is a minor check though. I hope the
> >>> >> >subject of this thread is making sense.
> >>> >> >
> >>> >> >VLAN ranges seem to have been included for an UX purpose (so commands
> >>> >> >look like Cisco IOS). We don't want to change any existing interface,
> >>> >> >so
> >>> >> >we pushed that down to drivers, with the only valid reason that, maybe
> >>> >> >one day, an hardware can be capable of programming a range on a
> >>> >> >per-port
> >>> >> >basis.
> >>> >> Hi,
> >>> >>
> >>> >> That's actually what we are doing in mlxsw. We can do up to 256
> >>> >> entries in
> >>> >> one go. We've yet to submit this part.
> >>> >
> >>> >Perfect Ido, thanks for pointing this out! I'm OK with the range then.
> >>> >
> >>> >So there is now a very last question in my head for this, which is more
> >>> >a matter of kernel design. Should the user be aware of such underlying
> >>> >support? In other words, would it make sense to do this in a driver:
> >>> >
> >>> >foo_port_vlan_add(struct net_device *dev,
> >>> > struct switchdev_obj_port_vlan *vlan)
> >>> >{
> >>> >if (vlan->vid_begin != vlan->vid_end)
> >>> >return -ENOTSUPP; /* or something more relevant for user */
> >>> >
> >>> >return foo_port_single_vlan_add(dev, vlan->vid_begin);
> >>> >}
> >>> >
> >>> >So drivers keep being simple, and we can easily propagate the fact that
> >>> >one-or-all VLAN is not supportable, vs. the VLAN feature itself is not
> >>> >implemented and must be done in software.
> >>> I think that if you want to keep it simple, then Scott's advice from the
> >>> previous thread is the most appropriate one. I believe the hardware you
> >>> are using is simply not meant to support multiple 802.1Q bridges.
> >>
> >> You mean allowing only one Linux bridge over an hardware switch?
> >>
> >> It would for sure simplify how, as developers and users, we represent a
> >> physical switch. But I am not sure how to achieve that and I don't have
> >> strong opinions on this TBH.
> >
> >Hi Vivien, I think it's possible to keep switch ports on just one
> >bridge if we do a little bit of work on the NETDEV_CHANGEUPPER
> >notifier. This will give you the driver-level control you want. Do
> >you have time to investigate? The idea is:
> >
> >1) In your driver's handler for NETDEV_CHANGEUPPER, if switch port is
> >being added to a second bridge,then return NOTIFY_BAD. Your driver
> >needs to track the bridge count.
> >
> >2) In __netdev_upper_dev_link(), check the return code from the
> >call_netdevice_notifiers_info(NETDEV_CHANGEUPPER, ...) call, and if
> >NOTIFY_BAD, abort the linking operation (goto rollback_xxx).
> >
> Hi,
>
> We are doing something similar in mlxsw (not upstream yet). Jiri
> introduced PRE_CHANGEUPPER, which is called from the function you
> mentioned, but before the linking operation (so that you don't need to
> rollback).
>
> If the notification is about a linking operation and the master is a
> bridge different than the current one, then NOTIFY_BAD is returned.
Great, I'll wait fo
Re: [PATCH net-next] switchdev: enforce no pvid flag in vlan ranges
Wed, Oct 14, 2015 at 08:14:24PM IDT, sfel...@gmail.com wrote:
>On Wed, Oct 14, 2015 at 8:25 AM, Vivien Didelot
> wrote:
>> On Oct. Wednesday 14 (42) 09:14 AM, Ido Schimmel wrote:
>>> Tue, Oct 13, 2015 at 05:32:26PM IDT, vivien.dide...@savoirfairelinux.com
>>> wrote:
>>> >On Oct. Tuesday 13 (42) 11:31 AM, Ido Schimmel wrote:
>>> >> Mon, Oct 12, 2015 at 08:36:25PM IDT, vivien.dide...@savoirfairelinux.com
>>> >> wrote:
>>> >> >Hi guys,
>>> >> >
>>> >> >On Oct. Monday 12 (42) 02:01 PM, Nikolay Aleksandrov wrote:
>>> >> >> From: Nikolay Aleksandrov
>>> >> >>
>>> >> >> We shouldn't allow BRIDGE_VLAN_INFO_PVID flag in VLAN ranges.
>>> >> >>
>>> >> >> Signed-off-by: Nikolay Aleksandrov
>>> >> >> ---
>>> >> >> net/switchdev/switchdev.c | 3 +++
>>> >> >> 1 file changed, 3 insertions(+)
>>> >> >>
>>> >> >> diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
>>> >> >> index 6e4a4f9ad927..256c596de896 100644
>>> >> >> --- a/net/switchdev/switchdev.c
>>> >> >> +++ b/net/switchdev/switchdev.c
>>> >> >> @@ -720,6 +720,9 @@ static int switchdev_port_br_afspec(struct
>>> >> >> net_device *dev,
>>> >> >> if (vlan.vid_begin)
>>> >> >> return -EINVAL;
>>> >> >> vlan.vid_begin = vinfo->vid;
>>> >> >> + /* don't allow range of pvids */
>>> >> >> + if (vlan.flags & BRIDGE_VLAN_INFO_PVID)
>>> >> >> + return -EINVAL;
>>> >> >> } else if (vinfo->flags & BRIDGE_VLAN_INFO_RANGE_END)
>>> >> >> {
>>> >> >> if (!vlan.vid_begin)
>>> >> >> return -EINVAL;
>>> >> >> --
>>> >> >> 2.4.3
>>> >> >>
>>> >> >
>>> >> >Yes the patch looks good, but it is a minor check though. I hope the
>>> >> >subject of this thread is making sense.
>>> >> >
>>> >> >VLAN ranges seem to have been included for an UX purpose (so commands
>>> >> >look like Cisco IOS). We don't want to change any existing interface, so
>>> >> >we pushed that down to drivers, with the only valid reason that, maybe
>>> >> >one day, an hardware can be capable of programming a range on a per-port
>>> >> >basis.
>>> >> Hi,
>>> >>
>>> >> That's actually what we are doing in mlxsw. We can do up to 256 entries
>>> >> in
>>> >> one go. We've yet to submit this part.
>>> >
>>> >Perfect Ido, thanks for pointing this out! I'm OK with the range then.
>>> >
>>> >So there is now a very last question in my head for this, which is more
>>> >a matter of kernel design. Should the user be aware of such underlying
>>> >support? In other words, would it make sense to do this in a driver:
>>> >
>>> >foo_port_vlan_add(struct net_device *dev,
>>> > struct switchdev_obj_port_vlan *vlan)
>>> >{
>>> >if (vlan->vid_begin != vlan->vid_end)
>>> >return -ENOTSUPP; /* or something more relevant for user */
>>> >
>>> >return foo_port_single_vlan_add(dev, vlan->vid_begin);
>>> >}
>>> >
>>> >So drivers keep being simple, and we can easily propagate the fact that
>>> >one-or-all VLAN is not supportable, vs. the VLAN feature itself is not
>>> >implemented and must be done in software.
>>> I think that if you want to keep it simple, then Scott's advice from the
>>> previous thread is the most appropriate one. I believe the hardware you
>>> are using is simply not meant to support multiple 802.1Q bridges.
>>
>> You mean allowing only one Linux bridge over an hardware switch?
>>
>> It would for sure simplify how, as developers and users, we represent a
>> physical switch. But I am not sure how to achieve that and I don't have
>> strong opinions on this TBH.
>
>Hi Vivien, I think it's possible to keep switch ports on just one
>bridge if we do a little bit of work on the NETDEV_CHANGEUPPER
>notifier. This will give you the driver-level control you want. Do
>you have time to investigate? The idea is:
>
>1) In your driver's handler for NETDEV_CHANGEUPPER, if switch port is
>being added to a second bridge,then return NOTIFY_BAD. Your driver
>needs to track the bridge count.
>
>2) In __netdev_upper_dev_link(), check the return code from the
>call_netdevice_notifiers_info(NETDEV_CHANGEUPPER, ...) call, and if
>NOTIFY_BAD, abort the linking operation (goto rollback_xxx).
>
Hi,
We are doing something similar in mlxsw (not upstream yet). Jiri
introduced PRE_CHANGEUPPER, which is called from the function you
mentioned, but before the linking operation (so that you don't need to
rollback).
If the notification is about a linking operation and the master is a
bridge different than the current one, then NOTIFY_BAD is returned.
Vivien, regarding your WAN interface question, this is something we
currently don't do. We don't even flood traffic from bridged ports
to CPU (although we can), as it can saturate the bus. Only control
traffic is supposed to go there.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
Re: [PATCH net-next] switchdev: enforce no pvid flag in vlan ranges
On Wed, Oct 14, 2015 at 8:25 AM, Vivien Didelot
wrote:
> On Oct. Wednesday 14 (42) 09:14 AM, Ido Schimmel wrote:
>> Tue, Oct 13, 2015 at 05:32:26PM IDT, vivien.dide...@savoirfairelinux.com
>> wrote:
>> >On Oct. Tuesday 13 (42) 11:31 AM, Ido Schimmel wrote:
>> >> Mon, Oct 12, 2015 at 08:36:25PM IDT, vivien.dide...@savoirfairelinux.com
>> >> wrote:
>> >> >Hi guys,
>> >> >
>> >> >On Oct. Monday 12 (42) 02:01 PM, Nikolay Aleksandrov wrote:
>> >> >> From: Nikolay Aleksandrov
>> >> >>
>> >> >> We shouldn't allow BRIDGE_VLAN_INFO_PVID flag in VLAN ranges.
>> >> >>
>> >> >> Signed-off-by: Nikolay Aleksandrov
>> >> >> ---
>> >> >> net/switchdev/switchdev.c | 3 +++
>> >> >> 1 file changed, 3 insertions(+)
>> >> >>
>> >> >> diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
>> >> >> index 6e4a4f9ad927..256c596de896 100644
>> >> >> --- a/net/switchdev/switchdev.c
>> >> >> +++ b/net/switchdev/switchdev.c
>> >> >> @@ -720,6 +720,9 @@ static int switchdev_port_br_afspec(struct
>> >> >> net_device *dev,
>> >> >> if (vlan.vid_begin)
>> >> >> return -EINVAL;
>> >> >> vlan.vid_begin = vinfo->vid;
>> >> >> + /* don't allow range of pvids */
>> >> >> + if (vlan.flags & BRIDGE_VLAN_INFO_PVID)
>> >> >> + return -EINVAL;
>> >> >> } else if (vinfo->flags & BRIDGE_VLAN_INFO_RANGE_END) {
>> >> >> if (!vlan.vid_begin)
>> >> >> return -EINVAL;
>> >> >> --
>> >> >> 2.4.3
>> >> >>
>> >> >
>> >> >Yes the patch looks good, but it is a minor check though. I hope the
>> >> >subject of this thread is making sense.
>> >> >
>> >> >VLAN ranges seem to have been included for an UX purpose (so commands
>> >> >look like Cisco IOS). We don't want to change any existing interface, so
>> >> >we pushed that down to drivers, with the only valid reason that, maybe
>> >> >one day, an hardware can be capable of programming a range on a per-port
>> >> >basis.
>> >> Hi,
>> >>
>> >> That's actually what we are doing in mlxsw. We can do up to 256 entries in
>> >> one go. We've yet to submit this part.
>> >
>> >Perfect Ido, thanks for pointing this out! I'm OK with the range then.
>> >
>> >So there is now a very last question in my head for this, which is more
>> >a matter of kernel design. Should the user be aware of such underlying
>> >support? In other words, would it make sense to do this in a driver:
>> >
>> >foo_port_vlan_add(struct net_device *dev,
>> > struct switchdev_obj_port_vlan *vlan)
>> >{
>> >if (vlan->vid_begin != vlan->vid_end)
>> >return -ENOTSUPP; /* or something more relevant for user */
>> >
>> >return foo_port_single_vlan_add(dev, vlan->vid_begin);
>> >}
>> >
>> >So drivers keep being simple, and we can easily propagate the fact that
>> >one-or-all VLAN is not supportable, vs. the VLAN feature itself is not
>> >implemented and must be done in software.
>> I think that if you want to keep it simple, then Scott's advice from the
>> previous thread is the most appropriate one. I believe the hardware you
>> are using is simply not meant to support multiple 802.1Q bridges.
>
> You mean allowing only one Linux bridge over an hardware switch?
>
> It would for sure simplify how, as developers and users, we represent a
> physical switch. But I am not sure how to achieve that and I don't have
> strong opinions on this TBH.
Hi Vivien, I think it's possible to keep switch ports on just one
bridge if we do a little bit of work on the NETDEV_CHANGEUPPER
notifier. This will give you the driver-level control you want. Do
you have time to investigate? The idea is:
1) In your driver's handler for NETDEV_CHANGEUPPER, if switch port is
being added to a second bridge,then return NOTIFY_BAD. Your driver
needs to track the bridge count.
2) In __netdev_upper_dev_link(), check the return code from the
call_netdevice_notifiers_info(NETDEV_CHANGEUPPER, ...) call, and if
NOTIFY_BAD, abort the linking operation (goto rollback_xxx).
-scott
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net-next] switchdev: enforce no pvid flag in vlan ranges
On Oct. Wednesday 14 (42) 09:14 AM, Ido Schimmel wrote:
> Tue, Oct 13, 2015 at 05:32:26PM IDT, vivien.dide...@savoirfairelinux.com
> wrote:
> >On Oct. Tuesday 13 (42) 11:31 AM, Ido Schimmel wrote:
> >> Mon, Oct 12, 2015 at 08:36:25PM IDT, vivien.dide...@savoirfairelinux.com
> >> wrote:
> >> >Hi guys,
> >> >
> >> >On Oct. Monday 12 (42) 02:01 PM, Nikolay Aleksandrov wrote:
> >> >> From: Nikolay Aleksandrov
> >> >>
> >> >> We shouldn't allow BRIDGE_VLAN_INFO_PVID flag in VLAN ranges.
> >> >>
> >> >> Signed-off-by: Nikolay Aleksandrov
> >> >> ---
> >> >> net/switchdev/switchdev.c | 3 +++
> >> >> 1 file changed, 3 insertions(+)
> >> >>
> >> >> diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
> >> >> index 6e4a4f9ad927..256c596de896 100644
> >> >> --- a/net/switchdev/switchdev.c
> >> >> +++ b/net/switchdev/switchdev.c
> >> >> @@ -720,6 +720,9 @@ static int switchdev_port_br_afspec(struct
> >> >> net_device *dev,
> >> >> if (vlan.vid_begin)
> >> >> return -EINVAL;
> >> >> vlan.vid_begin = vinfo->vid;
> >> >> + /* don't allow range of pvids */
> >> >> + if (vlan.flags & BRIDGE_VLAN_INFO_PVID)
> >> >> + return -EINVAL;
> >> >> } else if (vinfo->flags & BRIDGE_VLAN_INFO_RANGE_END) {
> >> >> if (!vlan.vid_begin)
> >> >> return -EINVAL;
> >> >> --
> >> >> 2.4.3
> >> >>
> >> >
> >> >Yes the patch looks good, but it is a minor check though. I hope the
> >> >subject of this thread is making sense.
> >> >
> >> >VLAN ranges seem to have been included for an UX purpose (so commands
> >> >look like Cisco IOS). We don't want to change any existing interface, so
> >> >we pushed that down to drivers, with the only valid reason that, maybe
> >> >one day, an hardware can be capable of programming a range on a per-port
> >> >basis.
> >> Hi,
> >>
> >> That's actually what we are doing in mlxsw. We can do up to 256 entries in
> >> one go. We've yet to submit this part.
> >
> >Perfect Ido, thanks for pointing this out! I'm OK with the range then.
> >
> >So there is now a very last question in my head for this, which is more
> >a matter of kernel design. Should the user be aware of such underlying
> >support? In other words, would it make sense to do this in a driver:
> >
> >foo_port_vlan_add(struct net_device *dev,
> > struct switchdev_obj_port_vlan *vlan)
> >{
> >if (vlan->vid_begin != vlan->vid_end)
> >return -ENOTSUPP; /* or something more relevant for user */
> >
> >return foo_port_single_vlan_add(dev, vlan->vid_begin);
> >}
> >
> >So drivers keep being simple, and we can easily propagate the fact that
> >one-or-all VLAN is not supportable, vs. the VLAN feature itself is not
> >implemented and must be done in software.
> I think that if you want to keep it simple, then Scott's advice from the
> previous thread is the most appropriate one. I believe the hardware you
> are using is simply not meant to support multiple 802.1Q bridges.
You mean allowing only one Linux bridge over an hardware switch?
It would for sure simplify how, as developers and users, we represent a
physical switch. But I am not sure how to achieve that and I don't have
strong opinions on this TBH.
> Trying to go around this will simply result in weird behavior (such as
> not supporting VLAN ranges), which is only sacrificed to support
> something a user doesn't even require (given the fact he's aware the
> hardware is meant to support only one 802.1Q bridge).
>
> What do you think?
I think mapping one Linux bridge to one physical switch (and thus have
real bridge device) makes a lot of sense.
But since Linux soft bridges are not stackable (yet?), how do we join 2
distinct interfaces then? e.g. eth1 is WAN and switch ports behind eth0.
> >
> >I'm not sure how transparent the hardware must be to the user.
> >
> >(the problem of one VLAN unsupported in a range is still something we
> >need to address).
> >
> >>
> >> >
> >> >So what happens is that we'll add some code to fix and check non-sense
> >> >(e.g. range + PVID) in switchdev, bridge, and I'm sure we are missing
> >> >other spots.
> >> >
> >> >Sorry for being insistent, but this still doesn't look right to me.
> >> >
> >> >It seems like we are bloating bridge, switchdev and drivers for the only
> >> >reason to maintain a kernel support for something like:
> >> >
> >> ># for i in $(seq 100 4000); do bridge vlan add vid $i dev swp0; done
Thanks,
-v
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net-next] switchdev: enforce no pvid flag in vlan ranges
Tue, Oct 13, 2015 at 05:32:26PM IDT, vivien.dide...@savoirfairelinux.com wrote:
>On Oct. Tuesday 13 (42) 11:31 AM, Ido Schimmel wrote:
>> Mon, Oct 12, 2015 at 08:36:25PM IDT, vivien.dide...@savoirfairelinux.com
>> wrote:
>> >Hi guys,
>> >
>> >On Oct. Monday 12 (42) 02:01 PM, Nikolay Aleksandrov wrote:
>> >> From: Nikolay Aleksandrov
>> >>
>> >> We shouldn't allow BRIDGE_VLAN_INFO_PVID flag in VLAN ranges.
>> >>
>> >> Signed-off-by: Nikolay Aleksandrov
>> >> ---
>> >> net/switchdev/switchdev.c | 3 +++
>> >> 1 file changed, 3 insertions(+)
>> >>
>> >> diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
>> >> index 6e4a4f9ad927..256c596de896 100644
>> >> --- a/net/switchdev/switchdev.c
>> >> +++ b/net/switchdev/switchdev.c
>> >> @@ -720,6 +720,9 @@ static int switchdev_port_br_afspec(struct net_device
>> >> *dev,
>> >> if (vlan.vid_begin)
>> >> return -EINVAL;
>> >> vlan.vid_begin = vinfo->vid;
>> >> + /* don't allow range of pvids */
>> >> + if (vlan.flags & BRIDGE_VLAN_INFO_PVID)
>> >> + return -EINVAL;
>> >> } else if (vinfo->flags & BRIDGE_VLAN_INFO_RANGE_END) {
>> >> if (!vlan.vid_begin)
>> >> return -EINVAL;
>> >> --
>> >> 2.4.3
>> >>
>> >
>> >Yes the patch looks good, but it is a minor check though. I hope the
>> >subject of this thread is making sense.
>> >
>> >VLAN ranges seem to have been included for an UX purpose (so commands
>> >look like Cisco IOS). We don't want to change any existing interface, so
>> >we pushed that down to drivers, with the only valid reason that, maybe
>> >one day, an hardware can be capable of programming a range on a per-port
>> >basis.
>> Hi,
>>
>> That's actually what we are doing in mlxsw. We can do up to 256 entries in
>> one go. We've yet to submit this part.
>
>Perfect Ido, thanks for pointing this out! I'm OK with the range then.
>
>So there is now a very last question in my head for this, which is more
>a matter of kernel design. Should the user be aware of such underlying
>support? In other words, would it make sense to do this in a driver:
>
>foo_port_vlan_add(struct net_device *dev,
> struct switchdev_obj_port_vlan *vlan)
>{
>if (vlan->vid_begin != vlan->vid_end)
>return -ENOTSUPP; /* or something more relevant for user */
>
>return foo_port_single_vlan_add(dev, vlan->vid_begin);
>}
>
>So drivers keep being simple, and we can easily propagate the fact that
>one-or-all VLAN is not supportable, vs. the VLAN feature itself is not
>implemented and must be done in software.
I think that if you want to keep it simple, then Scott's advice from the
previous thread is the most appropriate one. I believe the hardware you
are using is simply not meant to support multiple 802.1Q bridges.
Trying to go around this will simply result in weird behavior (such as
not supporting VLAN ranges), which is only sacrificed to support
something a user doesn't even require (given the fact he's aware the
hardware is meant to support only one 802.1Q bridge).
What do you think?
>
>I'm not sure how transparent the hardware must be to the user.
>
>(the problem of one VLAN unsupported in a range is still something we
>need to address).
>
>>
>> >
>> >So what happens is that we'll add some code to fix and check non-sense
>> >(e.g. range + PVID) in switchdev, bridge, and I'm sure we are missing
>> >other spots.
>> >
>> >Sorry for being insistent, but this still doesn't look right to me.
>> >
>> >It seems like we are bloating bridge, switchdev and drivers for the only
>> >reason to maintain a kernel support for something like:
>> >
>> ># for i in $(seq 100 4000); do bridge vlan add vid $i dev swp0; done
>
>Thanks,
>-v
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net-next] switchdev: enforce no pvid flag in vlan ranges
On Oct. Tuesday 13 (42) 11:31 AM, Ido Schimmel wrote:
> Mon, Oct 12, 2015 at 08:36:25PM IDT, vivien.dide...@savoirfairelinux.com
> wrote:
> >Hi guys,
> >
> >On Oct. Monday 12 (42) 02:01 PM, Nikolay Aleksandrov wrote:
> >> From: Nikolay Aleksandrov
> >>
> >> We shouldn't allow BRIDGE_VLAN_INFO_PVID flag in VLAN ranges.
> >>
> >> Signed-off-by: Nikolay Aleksandrov
> >> ---
> >> net/switchdev/switchdev.c | 3 +++
> >> 1 file changed, 3 insertions(+)
> >>
> >> diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
> >> index 6e4a4f9ad927..256c596de896 100644
> >> --- a/net/switchdev/switchdev.c
> >> +++ b/net/switchdev/switchdev.c
> >> @@ -720,6 +720,9 @@ static int switchdev_port_br_afspec(struct net_device
> >> *dev,
> >>if (vlan.vid_begin)
> >>return -EINVAL;
> >>vlan.vid_begin = vinfo->vid;
> >> + /* don't allow range of pvids */
> >> + if (vlan.flags & BRIDGE_VLAN_INFO_PVID)
> >> + return -EINVAL;
> >>} else if (vinfo->flags & BRIDGE_VLAN_INFO_RANGE_END) {
> >>if (!vlan.vid_begin)
> >>return -EINVAL;
> >> --
> >> 2.4.3
> >>
> >
> >Yes the patch looks good, but it is a minor check though. I hope the
> >subject of this thread is making sense.
> >
> >VLAN ranges seem to have been included for an UX purpose (so commands
> >look like Cisco IOS). We don't want to change any existing interface, so
> >we pushed that down to drivers, with the only valid reason that, maybe
> >one day, an hardware can be capable of programming a range on a per-port
> >basis.
> Hi,
>
> That's actually what we are doing in mlxsw. We can do up to 256 entries in
> one go. We've yet to submit this part.
Perfect Ido, thanks for pointing this out! I'm OK with the range then.
So there is now a very last question in my head for this, which is more
a matter of kernel design. Should the user be aware of such underlying
support? In other words, would it make sense to do this in a driver:
foo_port_vlan_add(struct net_device *dev,
struct switchdev_obj_port_vlan *vlan)
{
if (vlan->vid_begin != vlan->vid_end)
return -ENOTSUPP; /* or something more relevant for user */
return foo_port_single_vlan_add(dev, vlan->vid_begin);
}
So drivers keep being simple, and we can easily propagate the fact that
one-or-all VLAN is not supportable, vs. the VLAN feature itself is not
implemented and must be done in software.
I'm not sure how transparent the hardware must be to the user.
(the problem of one VLAN unsupported in a range is still something we
need to address).
>
> >
> >So what happens is that we'll add some code to fix and check non-sense
> >(e.g. range + PVID) in switchdev, bridge, and I'm sure we are missing
> >other spots.
> >
> >Sorry for being insistent, but this still doesn't look right to me.
> >
> >It seems like we are bloating bridge, switchdev and drivers for the only
> >reason to maintain a kernel support for something like:
> >
> ># for i in $(seq 100 4000); do bridge vlan add vid $i dev swp0; done
Thanks,
-v
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net-next] switchdev: enforce no pvid flag in vlan ranges
From: Nikolay Aleksandrov Date: Mon, 12 Oct 2015 14:01:39 +0200 > From: Nikolay Aleksandrov > > We shouldn't allow BRIDGE_VLAN_INFO_PVID flag in VLAN ranges. > > Signed-off-by: Nikolay Aleksandrov Applied, thanks. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net-next] switchdev: enforce no pvid flag in vlan ranges
Mon, Oct 12, 2015 at 08:36:25PM IDT, vivien.dide...@savoirfairelinux.com wrote:
>Hi guys,
>
>On Oct. Monday 12 (42) 02:01 PM, Nikolay Aleksandrov wrote:
>> From: Nikolay Aleksandrov
>>
>> We shouldn't allow BRIDGE_VLAN_INFO_PVID flag in VLAN ranges.
>>
>> Signed-off-by: Nikolay Aleksandrov
>> ---
>> net/switchdev/switchdev.c | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
>> index 6e4a4f9ad927..256c596de896 100644
>> --- a/net/switchdev/switchdev.c
>> +++ b/net/switchdev/switchdev.c
>> @@ -720,6 +720,9 @@ static int switchdev_port_br_afspec(struct net_device
>> *dev,
>> if (vlan.vid_begin)
>> return -EINVAL;
>> vlan.vid_begin = vinfo->vid;
>> +/* don't allow range of pvids */
>> +if (vlan.flags & BRIDGE_VLAN_INFO_PVID)
>> +return -EINVAL;
>> } else if (vinfo->flags & BRIDGE_VLAN_INFO_RANGE_END) {
>> if (!vlan.vid_begin)
>> return -EINVAL;
>> --
>> 2.4.3
>>
>
>Yes the patch looks good, but it is a minor check though. I hope the
>subject of this thread is making sense.
>
>VLAN ranges seem to have been included for an UX purpose (so commands
>look like Cisco IOS). We don't want to change any existing interface, so
>we pushed that down to drivers, with the only valid reason that, maybe
>one day, an hardware can be capable of programming a range on a per-port
>basis.
Hi,
That's actually what we are doing in mlxsw. We can do up to 256 entries in
one go. We've yet to submit this part.
>
>So what happens is that we'll add some code to fix and check non-sense
>(e.g. range + PVID) in switchdev, bridge, and I'm sure we are missing
>other spots.
>
>Sorry for being insistent, but this still doesn't look right to me.
>
>It seems like we are bloating bridge, switchdev and drivers for the only
>reason to maintain a kernel support for something like:
>
># for i in $(seq 100 4000); do bridge vlan add vid $i dev swp0; done
>
>Thanks,
>-v
>--
>To unsubscribe from this list: send the line "unsubscribe netdev" in
>the body of a message to majord...@vger.kernel.org
>More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net-next] switchdev: enforce no pvid flag in vlan ranges
On Mon, Oct 12, 2015 at 10:36 AM, Vivien Didelot
wrote:
> Hi guys,
>
> On Oct. Monday 12 (42) 02:01 PM, Nikolay Aleksandrov wrote:
>> From: Nikolay Aleksandrov
>>
>> We shouldn't allow BRIDGE_VLAN_INFO_PVID flag in VLAN ranges.
>>
>> Signed-off-by: Nikolay Aleksandrov
>> ---
>> net/switchdev/switchdev.c | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
>> index 6e4a4f9ad927..256c596de896 100644
>> --- a/net/switchdev/switchdev.c
>> +++ b/net/switchdev/switchdev.c
>> @@ -720,6 +720,9 @@ static int switchdev_port_br_afspec(struct net_device
>> *dev,
>> if (vlan.vid_begin)
>> return -EINVAL;
>> vlan.vid_begin = vinfo->vid;
>> + /* don't allow range of pvids */
>> + if (vlan.flags & BRIDGE_VLAN_INFO_PVID)
>> + return -EINVAL;
>> } else if (vinfo->flags & BRIDGE_VLAN_INFO_RANGE_END) {
>> if (!vlan.vid_begin)
>> return -EINVAL;
>> --
>> 2.4.3
>>
>
> Yes the patch looks good, but it is a minor check though. I hope the
> subject of this thread is making sense.
>
> VLAN ranges seem to have been included for an UX purpose (so commands
> look like Cisco IOS). We don't want to change any existing interface, so
> we pushed that down to drivers, with the only valid reason that, maybe
> one day, an hardware can be capable of programming a range on a per-port
> basis.
>
> So what happens is that we'll add some code to fix and check non-sense
> (e.g. range + PVID) in switchdev, bridge, and I'm sure we are missing
> other spots.
>
> Sorry for being insistent, but this still doesn't look right to me.
>
> It seems like we are bloating bridge, switchdev and drivers for the only
> reason to maintain a kernel support for something like:
>
> # for i in $(seq 100 4000); do bridge vlan add vid $i dev swp0; done
[Adding Roopa]
(Roopa, correct me if I'm wrong).
Ref: commit # bdced7ef
IIRC, vlan ranges were introduced to reduce the volume of netlink
traffic when doing a command like your's above. Your command would
create 3901 netlink msgs with a single IFLA_BRIDGE_VLAN_INFO, sent to
the kernel and then echoed back to user-space. With vlan ranges, the
cmd "bridge vlan add vid 100-4000 dev swp0" would result in one
netlink msg with one IFLA_BRIDGE_VLAN_INFO. Seems there were problems
with a getlink dump over-running the netlink msg (not using
NLM_MULTI)? Maybe Roopa can expand on the problem that was solved.
So the bridge command, and everything below it, has this feature. We
can't undo it...it's in the wild, so to speak And, as mentioned
before, it's and all-or-nothing command, and the underlying switchdev
or driver code needs to deal with overlapping vlan maps.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net-next] switchdev: enforce no pvid flag in vlan ranges
Hi guys,
On Oct. Monday 12 (42) 02:01 PM, Nikolay Aleksandrov wrote:
> From: Nikolay Aleksandrov
>
> We shouldn't allow BRIDGE_VLAN_INFO_PVID flag in VLAN ranges.
>
> Signed-off-by: Nikolay Aleksandrov
> ---
> net/switchdev/switchdev.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
> index 6e4a4f9ad927..256c596de896 100644
> --- a/net/switchdev/switchdev.c
> +++ b/net/switchdev/switchdev.c
> @@ -720,6 +720,9 @@ static int switchdev_port_br_afspec(struct net_device
> *dev,
> if (vlan.vid_begin)
> return -EINVAL;
> vlan.vid_begin = vinfo->vid;
> + /* don't allow range of pvids */
> + if (vlan.flags & BRIDGE_VLAN_INFO_PVID)
> + return -EINVAL;
> } else if (vinfo->flags & BRIDGE_VLAN_INFO_RANGE_END) {
> if (!vlan.vid_begin)
> return -EINVAL;
> --
> 2.4.3
>
Yes the patch looks good, but it is a minor check though. I hope the
subject of this thread is making sense.
VLAN ranges seem to have been included for an UX purpose (so commands
look like Cisco IOS). We don't want to change any existing interface, so
we pushed that down to drivers, with the only valid reason that, maybe
one day, an hardware can be capable of programming a range on a per-port
basis.
So what happens is that we'll add some code to fix and check non-sense
(e.g. range + PVID) in switchdev, bridge, and I'm sure we are missing
other spots.
Sorry for being insistent, but this still doesn't look right to me.
It seems like we are bloating bridge, switchdev and drivers for the only
reason to maintain a kernel support for something like:
# for i in $(seq 100 4000); do bridge vlan add vid $i dev swp0; done
Thanks,
-v
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net-next] switchdev: enforce no pvid flag in vlan ranges
> On Oct 12, 2015, at 3:01 PM, Nikolay Aleksandrov wrote:
>
> From: Nikolay Aleksandrov
>
> We shouldn't allow BRIDGE_VLAN_INFO_PVID flag in VLAN ranges.
>
> Signed-off-by: Nikolay Aleksandrov
> ---
> net/switchdev/switchdev.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
> index 6e4a4f9ad927..256c596de896 100644
> --- a/net/switchdev/switchdev.c
> +++ b/net/switchdev/switchdev.c
> @@ -720,6 +720,9 @@ static int switchdev_port_br_afspec(struct net_device
> *dev,
> if (vlan.vid_begin)
> return -EINVAL;
> vlan.vid_begin = vinfo->vid;
> + /* don't allow range of pvids */
> + if (vlan.flags & BRIDGE_VLAN_INFO_PVID)
> + return -EINVAL;
> } else if (vinfo->flags & BRIDGE_VLAN_INFO_RANGE_END) {
> if (!vlan.vid_begin)
> return -EINVAL;
Acked-by: Elad Raz
> --
> 2.4.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net-next] switchdev: enforce no pvid flag in vlan ranges
Mon, Oct 12, 2015 at 02:01:39PM CEST, ra...@blackwall.org wrote: >From: Nikolay Aleksandrov > >We shouldn't allow BRIDGE_VLAN_INFO_PVID flag in VLAN ranges. > >Signed-off-by: Nikolay Aleksandrov Acked-by: Jiri Pirko -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
