netdev - Re: [PATCH net-next 1/3] iavf: add check for current MAC address in set

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: <b7b63c6b-7bfb-6bd7-e361-298da38011a4@intel.com>
Date: Wed, 7 Jun 2023 12:29:36 +0200
From: Piotr Gardocki <piotrx.gardocki@...el.com>
To: Jakub Kicinski <kuba@...nel.org>, Maciej Fijalkowski
	<maciej.fijalkowski@...el.com>
CC: Tony Nguyen <anthony.l.nguyen@...el.com>, <davem@...emloft.net>,
	<pabeni@...hat.com>, <edumazet@...gle.com>, <netdev@...r.kernel.org>, "Michal
 Swiatkowski" <michal.swiatkowski@...ux.intel.com>, Rafal Romanowski
	<rafal.romanowski@...el.com>, <aleksander.lobakin@...el.com>
Subject: Re: [PATCH net-next 1/3] iavf: add check for current MAC address in
 set_mac callback

On 06.06.2023 19:24, Jakub Kicinski wrote:
> On Tue, 6 Jun 2023 12:21:07 +0200 Maciej Fijalkowski wrote:
>>>> couldn't this be checked the layer above then? and pulled out of drivers?
>>>
>>> Probably it could, but I can't tell for all drivers if such request should
>>> always be ignored. I'm not aware of all possible use cases for this callback
>>> to be called and I can imagine designs where such request should be
>>> always handled.  
>>
>> if you can imagine a case where such request should be handled then i'm
>> all ears. it feels like this is in an optimization where everyone could
>> benefit from (no expert in this scope though), but yeah this callback went
>> into the wild and it's implemented all over the place.
> 
> +1, FWIW, this is a net-next change, let's try to put it in the core
> unless we see a clear enough reason not to.

The reason I was not keen to move it to core is that a lot of drivers actually
perform some actions even when the MAC address doesn't change, like writing
to HW registers, performing link down, link up routine, notifying physical
function drivers, etc. I'm not aware if any of them really need this kind of logic,
so I can't stand strongly against new proposal. If the community is fine with it
we can move it to core code.

I need a piece of advice though:
1) Should I fix it in this patch set, or treat it as a separate thread?
2) I suppose the change is required only in dev_set_mac_address function, but
am I right assuming we should do it before call to dev_pre_changeaddr_notify
and return from function early? What about call to add_device_randomness?