netdev - Re: [PATCH net-next 09/13] bridge: mcast: Add MDB get support

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] [day] [month] [year] [list]

Message-ID: <bb71b086-b60e-c130-8484-5a71d0a07f19@blackwall.org>
Date: Tue, 17 Oct 2023 15:53:05 +0300
From: Nikolay Aleksandrov <razor@...ckwall.org>
To: Ido Schimmel <idosch@...dia.com>
Cc: netdev@...r.kernel.org, bridge@...ts.linux-foundation.org,
 davem@...emloft.net, kuba@...nel.org, edumazet@...gle.com,
 pabeni@...hat.com, roopa@...dia.com, mlxsw@...dia.com
Subject: Re: [PATCH net-next 09/13] bridge: mcast: Add MDB get support

On 10/17/23 14:03, Ido Schimmel wrote:
> On Tue, Oct 17, 2023 at 12:24:44PM +0300, Nikolay Aleksandrov wrote:
>> On 10/16/23 16:12, Ido Schimmel wrote:
>>> Implement support for MDB get operation by looking up a matching MDB
>>> entry, allocating the skb according to the entry's size and then filling
>>> in the response. The operation is performed under the bridge multicast
>>> lock to ensure that the entry does not change between the time the reply
>>> size is determined and when the reply is filled in.
>>>
>>> Signed-off-by: Ido Schimmel <idosch@...dia.com>
>>> ---
>>>    net/bridge/br_device.c  |   1 +
>>>    net/bridge/br_mdb.c     | 154 ++++++++++++++++++++++++++++++++++++++++
>>>    net/bridge/br_private.h |   9 +++
>>>    3 files changed, 164 insertions(+)
>>>
>> [snip]
>>> +int br_mdb_get(struct net_device *dev, struct nlattr *tb[], u32 portid, u32 seq,
>>> +	       struct netlink_ext_ack *extack)
>>> +{
>>> +	struct net_bridge *br = netdev_priv(dev);
>>> +	struct net_bridge_mdb_entry *mp;
>>> +	struct sk_buff *skb;
>>> +	struct br_ip group;
>>> +	int err;
>>> +
>>> +	err = br_mdb_get_parse(dev, tb, &group, extack);
>>> +	if (err)
>>> +		return err;
>>> +
>>> +	spin_lock_bh(&br->multicast_lock);
>>
>> Since this is only reading, could we use rcu to avoid blocking mcast
>> processing?
> 
> I tried to explain this choice in the commit message. Do you think it's
> a non-issue?
> 

Unless you really need a stable snapshot, I think it's worth
not blocking igmp processing for a read. It's not critical,
if you do need a stable snapshot then it's ok.

>>
>>> +
>>> +	mp = br_mdb_ip_get(br, &group);
>>> +	if (!mp) {
>>> +		NL_SET_ERR_MSG_MOD(extack, "MDB entry not found");
>>> +		err = -ENOENT;
>>> +		goto unlock;
>>> +	}
>>> +
>>> +	skb = br_mdb_get_reply_alloc(mp);
>>> +	if (!skb) {
>>> +		err = -ENOMEM;
>>> +		goto unlock;
>>> +	}
>>> +
>>> +	err = br_mdb_get_reply_fill(skb, mp, portid, seq);
>>> +	if (err) {
>>> +		NL_SET_ERR_MSG_MOD(extack, "Failed to fill MDB get reply");
>>> +		goto free;
>>> +	}
>>> +
>>> +	spin_unlock_bh(&br->multicast_lock);
>>> +
>>> +	return rtnl_unicast(skb, dev_net(dev), portid);
>>> +
>>> +free:
>>> +	kfree_skb(skb);
>>> +unlock:
>>> +	spin_unlock_bh(&br->multicast_lock);
>>> +	return err;
>>> +}
>>