netdev - Re: [PATCH net-next 7/8] net: switchdev: Replace port attr set SDO with a notification

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:   Sun, 24 Feb 2019 08:47:27 -0800
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Ido Schimmel <idosch@...lanox.com>
Cc:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>,
        open list <linux-kernel@...r.kernel.org>,
        "open list:STAGING SUBSYSTEM" <devel@...verdev.osuosl.org>,
        "moderated list:ETHERNET BRIDGE" <bridge@...ts.linux-foundation.org>,
        Jiri Pirko <jiri@...lanox.com>,
        "andrew@...n.ch" <andrew@...n.ch>,
        "vivien.didelot@...il.com" <vivien.didelot@...il.com>
Subject: Re: [PATCH net-next 7/8] net: switchdev: Replace port attr set SDO
 with a notification

Le 2/23/19 à 2:32 AM, Ido Schimmel a écrit :
> On Fri, Feb 22, 2019 at 03:59:25PM -0800, Florian Fainelli wrote:
>> Drop switchdev_ops.switchdev_port_attr_set. Drop the uses of this field
>> from all clients, which were migrated to use switchdev notification in
>> the previous patches.
>>
>> Add a new function switchdev_port_attr_notify() that sends the switchdev
>> notifications SWITCHDEV_PORT_ATTR_SET and takes care, depending on
>> SWITCHDEV_F_DEFER to call the blocking (process) or non-blocking
>> (atomic) notifier chain accordingly.
>>
>> Drop __switchdev_port_attr_set() and update switchdev_port_attr_set()
>> likewise.
>>
>> Signed-off-by: Florian Fainelli <f.fainelli@...il.com>
>> ---
>>  net/switchdev/switchdev.c | 96 +++++++++++----------------------------
>>  1 file changed, 26 insertions(+), 70 deletions(-)
>>
>> diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
>> index 94400f5b8e07..a1f16836ef89 100644
>> --- a/net/switchdev/switchdev.c
>> +++ b/net/switchdev/switchdev.c
>> @@ -174,81 +174,35 @@ static int switchdev_deferred_enqueue(struct net_device *dev,
>>  	return 0;
>>  }
>>  
>> -/**
>> - *	switchdev_port_attr_get - Get port attribute
> 
> Hmm, why do you remove it here? Can't you remove it in a separate patch?
> I thought we already got rid of it :p

Yes it should have been removed, looks like my previous series did not
that, I will send that separately.

> 
>> - *
>> - *	@dev: port device
>> - *	@attr: attribute to get
>> - */
>> -int switchdev_port_attr_get(struct net_device *dev, struct switchdev_attr *attr)
>> +static int switchdev_port_attr_notify(enum switchdev_notifier_type nt,
>> +				      struct net_device *dev,
>> +				      const struct switchdev_attr *attr,
>> +				      struct switchdev_trans *trans)
>>  {
>> -	const struct switchdev_ops *ops = dev->switchdev_ops;
>> -	struct net_device *lower_dev;
>> -	struct list_head *iter;
>> -	struct switchdev_attr first = {
>> -		.id = SWITCHDEV_ATTR_ID_UNDEFINED
>> -	};
>> -	int err = -EOPNOTSUPP;
>> +	int err;
>> +	int rc;
>>  
>> -	if (ops && ops->switchdev_port_attr_get)
>> -		return ops->switchdev_port_attr_get(dev, attr);
>> +	struct switchdev_notifier_port_attr_info attr_info = {
>> +		.attr = attr,
>> +		.trans = trans,
>> +		.handled = false,
>> +	};
>>  
>> -	if (attr->flags & SWITCHDEV_F_NO_RECURSE)
>> +	if (attr & SWITCHDEV_F_DEFER)
>> +		rc = call_switchdev_blocking_notifiers(nt, dev,
>> +						       &attr_info.info, NULL);
>> +	else
>> +		rc = call_switchdev_notifiers(nt, dev, &attr_info.info, NULL);
> 
> I don't believe this is needed. You're calling this function from
> switchdev_port_attr_set_now() which is always called from process
> context. switchdev_port_attr_set() takes care of that. Similar to
> switchdev_port_obj_add().

Except for net/bridge/br_switchdev.c when we check the bridge port's
flags support with PRE_BRIDGE_FLAGS. In that case we are executing from
the caller (atomic) context and we can't defer otherwise that trumps the
whole idea of being able to do a quick check and return that to the
caller that we cannot support specific flags. How would you recommend
approaching that?
-- 
Florian