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: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <54EAAEBC.6080609@roeck-us.net>
Date:	Sun, 22 Feb 2015 20:38:20 -0800
From:	Guenter Roeck <linux@...ck-us.net>
To:	Andrew Lunn <andrew@...n.ch>
CC:	Florian Fainelli <f.fainelli@...il.com>, netdev@...r.kernel.org,
	davem@...emloft.net, vivien.didelot@...oirfairelinux.com,
	jerome.oufella@...oirfairelinux.com, cphealy@...il.com
Subject: Re: [PATCH RFC 1/2] net: dsa: integrate with SWITCHDEV for HW bridging

On 02/22/2015 08:22 PM, Andrew Lunn wrote:
> On Sun, Feb 22, 2015 at 08:07:03PM -0800, Guenter Roeck wrote:
>> On 02/22/2015 07:14 PM, Andrew Lunn wrote:
>>> On Sun, Feb 22, 2015 at 06:20:44PM -0800, Guenter Roeck wrote:
>>>> On 02/17/2015 11:26 AM, Florian Fainelli wrote:
>>>>> In order to support bridging offloads in DSA switch drivers, select
>>>>> NET_SWITCHDEV to get access to the port_stp_update and parent_get_id
>>>>> NDOs that we are required to implement.
>>>>>
>>>>> To facilitate the integratation at the DSA driver level, we implement 3
>>>>> types of operations:
>>>>>
>>>>
>>>> Hi Florian,
>>>>
>>>>>
>>>>> +/* Return a bitmask of all ports being currently bridged. Note that on
>>>>> + * leave, the mask will still return the bitmask of ports currently bridged,
>>>>> + * prior to port removal, and this is exactly what we want.
>>>>> + */
>>>>> +static u32 dsa_slave_br_port_mask(struct dsa_switch *ds)
>>>>> +{
>>>>> +	unsigned int port;
>>>>> +	u32 mask = 0;
>>>>> +
>>>>> +	for (port = 0; port < DSA_MAX_PORTS; port++) {
>>>>> +		if (!((1 << port) & ds->phys_port_mask))
>>>>> +			continue;
>>>>> +
>>>>> +		if (ds->ports[port]->priv_flags & IFF_BRIDGE_PORT)
>>>>> +			mask |= 1 << port;
>>>>
>>>> Problem is that the function can be called through dsa_slave_netdevice_event
>>>> before the slave devices are fully initialized.
>>>>
>>>> After adding
>>>>
>>>> +               if (!ds->ports[port]) {
>>>> +                       netdev_err(bridge,
>>>> +                                  "No ports data for port %d, mask=0x%x\n",
>>>> +                                  port, ds->phys_port_mask);
>>>> +                       continue;
>>>> +               }
>>>>
>>>> and with some more debug messages added to dsa_switch_setup(), I see the following.
>>>>
>>>> [   14.187290] e1000e 0000:00:19.0 em1: [0]: Creating slave device for port 1(port1)
>>>> [   14.272605] e1000e 0000:00:19.0 em1: [0]: Creating slave device for port 2(port2)
>>>> [   14.353118] e1000e 0000:00:19.0 em1: [0]: Creating slave device for port 3(port3)
>>>> [   14.472002] br0: No ports data for port 3, mask=0x1e
>>>> [   14.472053] br0: No ports data for port 4, mask=0x1e
>>>> [   14.472753] e1000e 0000:00:19.0 em1: [0]: Creating slave device for port 4(host2esb)
>>>>
>>>> This happens if I add the bridge configuration to /etc/network/interfaces instead
>>>> of creating the bridge manually. Apparently dsa_switch_setup() is not yet complete
>>>> when dsa_slave_netdevice_event is executed to handle a state change on one of its
>>>> newly created slave interfaces.
>>>>
>>>> The relevant information from /etc/network/interfaces is:
>>>>
>>>> auto br0
>>>>
>>>> iface port1 inet manual
>>>> iface port2 inet manual
>>>>
>>>> iface br0 inet dhcp
>>>> 	bridge_ports port1 port2
>>>
>>> Hi Guenter
>>>
>>> Does this actually matter? The ports which don't exists yet are not
>>> being added to the bridge. The mask will come out correct. What
>>> happens when port4 is made a member of the bridge? I expect it
>>> works. It is the creation of the interface which triggers hotplug to
>>> read interfaces and add the interface to the port.
>>>
>>
>> 	if (!ds->ports[port])
>> 		continue;
>>
>> might be an option. However, I am not sure that what you say is correct,
>> at least not strictly speaking. dsa_slave_create() returns the created
>> slave device, which is added to ds->ports[port] in dsa_switch_setup().
>> Since there is no protection in dsa_switch_setup(), there is no guarantee
>> that the callback doesn't happen prior to the initialization of
>> ds->ports[port]. So the above would leave a race condition, where the
>> port being added to the bridge _is_ one for which ds->ports[port] is
>> not yet initialized.
>
> Yes, you are correct, there is a race here.
>
>
>> Protecting the entire slave creation loop in dsa_switch_setup()
>> and using register_netdevice() in dsa_slave_create() solves the problem
>> as far as I can see, I just don't know if it is an acceptable solution.
>
> Or:
>
> diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
> index 2173402d87e0..1aa120d6d0e4 100644
> --- a/net/dsa/dsa.c
> +++ b/net/dsa/dsa.c
> @@ -325,8 +325,6 @@ dsa_switch_setup(struct dsa_switch_tree *dst, int index,
>                                     index, i, pd->port_names[i]);
>                          continue;
>                  }
> -
> -               ds->ports[i] = slave_dev;
>          }
>
>   #ifdef CONFIG_NET_DSA_HWMON
> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
> index f23deadf42a0..d6004072a957 100644
> --- a/net/dsa/slave.c
> +++ b/net/dsa/slave.c
> @@ -669,12 +669,14 @@ dsa_slave_create(struct dsa_switch *ds, struct device *parent,
>                  free_netdev(slave_dev);
>                  return NULL;
>          }
> +       ds->ports[port] = slave_dev;
>
>          ret = register_netdev(slave_dev);
>          if (ret) {
>                  netdev_err(master, "error %d registering interface %s\n",
>                             ret, slave_dev->name);
>                  phy_disconnect(p->phy);
> +               ds->ports[port] = NULL;
>                  free_netdev(slave_dev);
>                  return NULL;
>          }
>
> Not tested. But the point being, ensure everything is setup before
> calling register_netdev().
>

That should work. I'll give it a try.

Guenter

--
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

Powered by blists - more mailing lists