netdev - Re: [PATCH net-next] net/core: ensure features get disabled on new lower devs

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: <56393127.4050406@redhat.com>
Date:	Tue, 03 Nov 2015 17:11:51 -0500
From:	Jarod Wilson <jarod@...hat.com>
To:	Alexander Duyck <alexander.duyck@...il.com>
CC:	linux-kernel@...r.kernel.org,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jay Vosburgh <j.vosburgh@...il.com>,
	Veaceslav Falico <vfalico@...il.com>,
	Andy Gospodarek <gospo@...ulusnetworks.com>,
	Jiri Pirko <jiri@...nulli.us>,
	Nikolay Aleksandrov <razor@...ckwall.org>,
	Michal Kubecek <mkubecek@...e.cz>, netdev@...r.kernel.org
Subject: Re: [PATCH net-next] net/core: ensure features get disabled on new
 lower devs

Alexander Duyck wrote:
> On 11/03/2015 12:36 PM, Jarod Wilson wrote:
>> With moving netdev_sync_lower_features() after the .ndo_set_features
>> calls, I neglected to verify that devices added *after* a flag had been
>> disabled on an upper device were properly added with that flag
>> disabled as
>> well. This currently happens, because we exit __netdev_update_features()
>> when we see dev->features == features for the upper dev. We can retain
>> the
>> optimization of leaving without calling .ndo_set_features with a bit of
>> tweaking and a goto here.
>>
>> Changing err to ret was somewhat arbitrary and makes the patch look more
>> involved, but seems to better fit the altered use.
...
>> + 	if (!ret) {
>> + 		dev->features = features;
>> + 		ret = 1;
>> + 	}
>> +
>
> I would take the "ret = 1;" out of the if statement and let it stay here
> by itself. Technically anything that traversed this path was returning 1
> previously so we probably want to retain that behavior.

Ah, that. I took a look at all the callers of __netdev_update_features, 
and most don't even check return value, the one that does 
(netdev_update_features) only cares if its zero or not zero, so I 
figured it didn't really matter here, but it would indeed return 2 now 
instead of 1, if it got that from ndo_set_features. For consistency's 
sake, I can respin and just always set ret = 1 though.

>> +sync_lower:
>> 	/* some features must be disabled on lower devices when disabled
>> 	 * on an upper device (think: bonding master or bridge)
>> 	 */
>> 	netdev_for_each_lower_dev(dev, lower, iter)
>> 		netdev_sync_lower_features(dev, lower, features);
>>
>> - 	if (!err)
>> - 		dev->features = features;
>
> You could just alter the if statement here to check for a non-zero ret
> value since you should have it as either 0 or 1. It shouldn't have any
> other values.
>
> That way you will have disabled the feature on the lower devices before
> advertising that it has been disabled on the upper device.

If this check is down here, the goto will trigger, setting dev->features 
= features, but then, we got there because dev->features == features 
already, so meh. But it would also NOT trigger in the case of 
ndo_set_features returning 0 anymore, because we set ret = 1. Or am I 
missing something or misunderstanding what you're suggesting here?

-- 
Jarod Wilson
jarod@...hat.com


--
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