netdev - Re: [PATCH net-next] rocker: check for BRIDGE_FLAGS

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <54F7A3D9.60805@cumulusnetworks.com>
Date:	Wed, 04 Mar 2015 16:31:21 -0800
From:	roopa <roopa@...ulusnetworks.com>
To:	Scott Feldman <sfeldma@...il.com>
CC:	Jiří Pírko <jiri@...nulli.us>,
	Netdev <netdev@...r.kernel.org>,
	"David S. Miller" <davem@...emloft.net>
Subject: Re: [PATCH net-next] rocker: check for BRIDGE_FLAGS_SELF in bridge
 setlink handler

On 3/4/15, 8:24 AM, Scott Feldman wrote:
> On Wed, Mar 4, 2015 at 12:51 AM, roopa <roopa@...ulusnetworks.com> wrote:
>> On 3/3/15, 11:02 PM, Scott Feldman wrote:
>>> On Tue, Mar 3, 2015 at 4:15 PM,  <roopa@...ulusnetworks.com> wrote:
>>>> From: Roopa Prabhu <roopa@...ulusnetworks.com>
>>>>
>>>> With the recent addition of the NETIF_F_HW_SWITCH_OFFLOAD flag
>>>> on rocker ports, the second command (bridge link set) below will turn off
>>>> learning in the rocker hw (Scott/Jiri, need some confirmation from
>>>> you that this is indeed a problem and if the below patch is ok).
>>>>
>>>> ip link set dev swp1 master br0
>>>> bridge link set dev swp1 learning off master
>>>> bridge link set dev swp1 learning_sync on self
>>>>
>>>> This patch fixes rocker to ignore learning setting when 'master'
>>>> is set. This makes it possible to set/unset learning in kernel and bridge
>>>> driver independently.
>>>>
>>>> The below command will continue to set learning on in both kernel and
>>>> rocker
>>>> hw:
>>>> bridge link set dev swp1 learning on
>>>>
>>>> Signed-off-by: Roopa Prabhu <roopa@...ulusnetworks.com>
>>>> ---
>>>>    drivers/net/ethernet/rocker/rocker.c |    3 +++
>>>>    1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/drivers/net/ethernet/rocker/rocker.c
>>>> b/drivers/net/ethernet/rocker/rocker.c
>>>> index e5a15a4..d7c31d2 100644
>>>> --- a/drivers/net/ethernet/rocker/rocker.c
>>>> +++ b/drivers/net/ethernet/rocker/rocker.c
>>>> @@ -3769,6 +3769,9 @@ static int rocker_port_bridge_setlink(struct
>>>> net_device *dev,
>>>>           struct nlattr *attr;
>>>>           int err;
>>>>
>>>> +       if (flags && !(flags & BRIDGE_FLAGS_SELF))
>>>> +               return 0;
>>>> +
>>>>           protinfo = nlmsg_find_attr(nlh, sizeof(struct ifinfomsg),
>>>>                                      IFLA_PROTINFO);
>>>>           if (protinfo) {
>>>
>>> NACK on this patch.  This is the problem with netlink creeping into
>>> ndo ops: it's too tempting to push work-arounds down to driver.
>> netlink has already crept into the driver. You do parse the netlink message
>> right below.
>> The patch actually does not touch the message. It is just using one of the
>> args already passed to the handler.
>>
>>> In this case, you're making the driver check to see if it's SELF when
>>> it's already SELF by definition.
>>>
>>> Rocker setlink wasn't broken prior to the NETIF_F_HW_SWITCH_OFFLOAD
>>> patches.
>>>    Now it is,
>> sure, I can submit a patch to remove the flag on rocker ports if thats what
>> you prefer.
> I don't want that either.  I haven't figured out why I need
> NETIF_F_HW_SWITCH_OFFLOAD at the moment, but if this is supposed to be
> a flag that's set on switchdev driver ports, then it should be set on
> rocker ports.
Its not mandatory. Only If you need it. If you want the user to drive 
rocker explicitly using 'self',
then you dont need the OFFLOAD flag.

>
>
>>> but this isn't the right fix.
>>>
>>> Can we revisit this so these two commands only hit MASTER:
>>>
>>>      bridge link set dev swp1 learning on
>>>      bridge link set dev swp1 learning on master
>>>
>>> And this one hits SELF:
>>>
>>>       bridge link set dev swp1 learning on self
>>
>> For the above to work you just need to remove the feature flag in the driver
>> (i can submit a patch).
>>
>> The reason why it is useful to have it the way it currently is:
>>
>> the setlink request does not always only contain the 'learning' flag.
>> It handles vlans too. I dont see rocker parsing vlans yet ie IFLA_AF_SPEC
>> (or did i miss it ?)
> Yes, you missed it.  See
> br_setlink->br_afspec->br_vlan_info->nbp_vlan_add->__vlan_add->__vlan_vid_add
ok thanks for the clarification. I thought you were using 
ndo_bridge_setlink to pass the vlan info to the driver (because that is 
one way to pass vlans for the vlan filtering bridge). And the switch 
port driver already implements ndo_bridge_setlink.

>
> The bridge is already calling into the driver to tell it which vlans
> are on which port.
>
> This works when doing vlans outside of bridge driver also.  One
> interface for both bridge and stand-alone vlans.
agreed, however, i would have thought you also need the vinfo flags 
(struct bridge_vlan_info).
>
> I see no reason to parse netlink in the driver's setlink/dellink other
> than getting the SELF flags.
hmm.., the driver is already doing this to parse learning and learning 
sync flags. And bridge setlink flags is passed in the ndo op.

Let me know if you have other ideas to fix this (one approach is to not 
advertise the OFFLOAD flag on rocker ports).

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html