netdev - Re: [PATCH v7 net-next 08/11] net: dsa: allow changing the tag protocol via the "tagging" device attribute

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]

Date:   Wed, 27 Jan 2021 17:51:00 -0800
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Jakub Kicinski <kuba@...nel.org>,
        Vladimir Oltean <olteanv@...il.com>
Cc:     "David S . Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
        Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Claudiu Manoil <claudiu.manoil@....com>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        Vladimir Oltean <vladimir.oltean@....com>,
        UNGLinuxDriver@...rochip.com
Subject: Re: [PATCH v7 net-next 08/11] net: dsa: allow changing the tag
 protocol via the "tagging" device attribute



On 1/27/2021 5:30 PM, Jakub Kicinski wrote:
> On Tue, 26 Jan 2021 00:03:30 +0200 Vladimir Oltean wrote:
>> From: Vladimir Oltean <vladimir.oltean@....com>
>>
>> Currently DSA exposes the following sysfs:
>> $ cat /sys/class/net/eno2/dsa/tagging
>> ocelot
>>
>> which is a read-only device attribute, introduced in the kernel as
>> commit 98cdb4807123 ("net: dsa: Expose tagging protocol to user-space"),
>> and used by libpcap since its commit 993db3800d7d ("Add support for DSA
>> link-layer types").
>>
>> It would be nice if we could extend this device attribute by making it
>> writable:
>> $ echo ocelot-8021q > /sys/class/net/eno2/dsa/tagging
>>
>> This is useful with DSA switches that can make use of more than one
>> tagging protocol. It may be useful in dsa_loop in the future too, to
>> perform offline testing of various taggers, or for changing between dsa
>> and edsa on Marvell switches, if that is desirable.
>>
>> In terms of implementation, drivers can now move their tagging protocol
>> configuration outside of .setup/.teardown, and into .set_tag_protocol
>> and .del_tag_protocol. The calling order is:
>>
>> .setup -> [.set_tag_protocol -> .del_tag_protocol]+ -> .teardown
>>
>> There was one more contract between the DSA framework and drivers, which
>> is that if a CPU port needs to account for the tagger overhead in its
>> MTU configuration, it must do that privately. Which means it needs the
>> information about what tagger it uses before we call its MTU
>> configuration function. That promise is still held.
>>
>> Writing to the tagging sysfs will first tear down the tagging protocol
>> for all switches in the tree attached to that DSA master, then will
>> attempt setup with the new tagger.
>>
>> Writing will fail quickly with -EOPNOTSUPP for drivers that don't
>> support .set_tag_protocol, since that is checked during the deletion
>> phase. It is assumed that all switches within the same DSA tree use the
>> same driver, and therefore either all have .set_tag_protocol implemented,
>> or none do.
>>
>> Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
>> Reviewed-by: Florian Fainelli <f.fainelli@...il.com>
> 
>> +const struct dsa_device_ops *dsa_find_tagger_by_name(const char *buf)
>> +{
>> +	const struct dsa_device_ops *ops = NULL;
>> +	struct dsa_tag_driver *dsa_tag_driver;
>> +
>> +	mutex_lock(&dsa_tag_drivers_lock);
>> +	list_for_each_entry(dsa_tag_driver, &dsa_tag_drivers_list, list) {
>> +		const struct dsa_device_ops *tmp = dsa_tag_driver->ops;
>> +
>> +		if (!sysfs_streq(buf, tmp->name))
>> +			continue;
>> +
>> +		ops = tmp;
>> +		break;
>> +	}
>> +	mutex_unlock(&dsa_tag_drivers_lock);
> 
> What's protecting from the tag driver unloading at this very moment?

Yes good point I missed that, too. The tag driver would need to get its
reference count incremented similarly to what dsa_tag_driver_get() does
and you would need to release the previous tag driver if successful in
grabbing the new one.
-- 
Florian