netdev - Re: [patch net-next v2 01/28] net: dev: Check CHANGEUPPER notifier return value

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: <20151203082500.GB3075@colbert.mtl.com>
Date:	Thu, 3 Dec 2015 10:25:00 +0200
From:	Ido Schimmel <idosch@...lanox.com>
To:	John Fastabend <john.fastabend@...il.com>
CC:	Jiri Pirko <jiri@...nulli.us>, <netdev@...r.kernel.org>,
	<davem@...emloft.net>, <eladr@...lanox.com>, <yotamg@...lanox.com>,
	<ogerlitz@...lanox.com>, <j.vosburgh@...il.com>,
	<vfalico@...il.com>, <gospo@...ulusnetworks.com>,
	<dsa@...ulusnetworks.com>, <nikolay@...ulusnetworks.com>,
	<pjonnala@...adcom.com>, <f.fainelli@...il.com>,
	<sfeldma@...il.com>, <roopa@...ulusnetworks.com>, <andrew@...n.ch>
Subject: Re: [patch net-next v2 01/28] net: dev: Check CHANGEUPPER notifier
 return value

Thu, Dec 03, 2015 at 01:20:50AM IST, john.fastabend@...il.com wrote:
>On 15-12-02 12:07 PM, Jiri Pirko wrote:
>> From: Ido Schimmel <idosch@...lanox.com>
>> 
>> switchdev drivers reflect the newly requested topology to hardware when
>> CHANGEUPPER is received, after software links were already formed.
>> However, the operation can fail and user will not be notified, as the
>> return value of the notifier is not checked.
>> 
>> Add this check and rollback software links if necessary.
>> 
>> Signed-off-by: Ido Schimmel <idosch@...lanox.com>
>> Signed-off-by: Jiri Pirko <jiri@...lanox.com>
>> ---
>> v1->v2:
>> -new patch
>> ---
>>  net/core/dev.c | 7 +++++--
>>  1 file changed, 5 insertions(+), 2 deletions(-)
>> 
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index 5df6cbc..df33f82 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -5490,8 +5490,11 @@ static int __netdev_upper_dev_link(struct net_device *dev,
>>  			goto rollback_lower_mesh;
>>  	}
>>  
>> -	call_netdevice_notifiers_info(NETDEV_CHANGEUPPER, dev,
>> -				      &changeupper_info.info);
>> +	ret = call_netdevice_notifiers_info(NETDEV_CHANGEUPPER, dev,
>> +					    &changeupper_info.info);
>> +	if (ret)
>> +		goto rollback_lower_mesh;
>> +
>
>hmm small nit (take it or leave it) but I think it would be more
>correct if this was
>
>	if (ret == NOTIFY_BAD)
>		goto rollback_lower_mesh;
>
>It seems that NOTIFY_DONE, NOTIFY_OK and NOTIFY_STOP_MASK would be
>valid return codes that don't indicate an error. However seeing I
>couldn't find any cases of NOTIFY_OK/NOTIFY_STOP_MASK from the
>CHANGEUPPER event it doesn't matter in practice.
>

Hi,

It's actually not a small nit. I forgot to call 'notifier_to_errno' and
then check its retrun value instead that of the notifier. This would
return 0 for NOTIFY_{DONE,OK,STOP_MASK} and -1 for NOTIFY_BAD.

Thank you.

>Thanks,
>John
>
>>  	return 0;
>>  
>>  rollback_lower_mesh:
>> 
>
--
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