netdev - Re: [PATCH v2 net-next 2/7] net: dsa: merge all bools of struct dsa

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: <20220105185627.vq6kgnw2yhbymiuc@skbuf>
Date:   Wed, 5 Jan 2022 18:56:28 +0000
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     Florian Fainelli <f.fainelli@...il.com>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>
Subject: Re: [PATCH v2 net-next 2/7] net: dsa: merge all bools of struct
 dsa_port into a single u8

On Wed, Jan 05, 2022 at 10:46:44AM -0800, Florian Fainelli wrote:
> On 1/5/22 10:39 AM, Vladimir Oltean wrote:
> > Hi Florian,
> > 
> > On Wed, Jan 05, 2022 at 10:30:54AM -0800, Florian Fainelli wrote:
> >> Reviewed-by: Florian Fainelli <f.fainelli@...il.com>
> > 
> > Thanks a lot for the review.
> > 
> > I'm a bit on the fence on this patch and the other one for dsa_switch.
> > The thing is that bit fields are not atomic in C89, so if we update any
> > of the flags inside dp or ds concurrently (like dp->vlan_filtering),
> > we're in trouble. Right now this isn't a problem, because most of the
> > flags are set either during probe, or during ds->ops->setup, or are
> > serialized by the rtnl_mutex in ways that are there to stay (switchdev
> > notifiers). That's why I didn't say anything about it. But it may be a
> > caveat to watch out for in the future. Do you think we need to do
> > something about it? A lock would not be necessary, strictly speaking.
> 
> I would probably start with a comment that describes these pitfalls, I
> wish we had a programmatic way to ensure that these flags would not be
> set dynamically and outside of the probe/setup path but that won't
> happen easily.

A comment is probably good.

> Should we be switching to a bitmask and bitmap helpers to be future proof?

We could have done that, and it would certainly raise the awareness a
bit more, but the reason I went with the bit fields at least in the
first place was to reduce the churn in drivers. Otherwise, sure, if
driver changes are on the table for this, we can even discuss about
adding more arguments to dsa_register_switch(). The amount of poking
that drivers do inside struct dsa_switch has grown, and sometimes it's
not even immediately clear which members of that structure are supposed
to be populated by whom and when. We could definitely just tell them to
provide their desired behavior as arguments (vlan_filtering_is_global,
needs_standalone_vlan_filtering, configure_vlan_while_not_filtering,
untag_bridge_pvid, assisted_learning_on_cpu_port, ageing_time_min,
ageing_time_max, phys_mii_mask, num_tx_queues, num_lag_ids, max_num_bridges)
and only DSA will update struct dsa_switch, and we could then control
races better that way. But the downside is that we'd have 11 extra
arguments to dsa_register_switch()....