netdev - Re: [RFC PATCH] core: Add ioctls to control device unicast hw addresses

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:	Tue, 26 Feb 2013 21:01:43 -0800
From:	John Fastabend <john.r.fastabend@...el.com>
To:	vyasevic@...hat.com
CC:	John Fastabend <john.fastabend@...il.com>,
	David Miller <davem@...emloft.net>, netdev@...r.kernel.org
Subject: Re: [RFC PATCH] core: Add ioctls to control device unicast hw addresses

On 2/26/2013 6:27 PM, Vlad Yasevich wrote:
> On 02/26/2013 09:07 PM, John Fastabend wrote:
>> On Tue, Feb 26, 2013 at 05:15:03PM -0500, David Miller wrote:
>>> From: John Fastabend <john.r.fastabend@...el.com>
>>> Date: Tue, 26 Feb 2013 13:58:39 -0800
>>>
>>>> [...]
>>>>
>>>>>>>
>>>>>>>
>>>>>>> [RFC PATCH] rtnetlink: Add support for adding/removing additional hw
>>>>>>> addresses.
>>>>>>>
>>>>>>> Add an ability to add and remove HW addresses to the device
>>>>>>> unicast and multicast address lists.  Right now, we only have
>>>>>>> an ioctl() to manage the multicast addresses and there is no
>>>>>>> way the manage the unicast list.
>>>>>>>
>>>>>>> Signed-off-by: Vlad Yasevich <vyasevic@...hat.com>
>>>>>>
>>>>>> This is a step in the right direction, and you're right that there is
>>>>>> a difficulty in detecting whether support exists or not.
>>>>>>
>>>>>> I am so surprised that we've have ->set_rx_mode() support for
>>>>>> multiple
>>>>>> unicast MAC addresses in so many drivers all this time, yet no way
>>>>>> outside of FDB to make use of it at all.
>>>>>
>>>>> And even that is not always available.  In most drivers it requires
>>>>> module parameters or other explicit configuration steps.  Meanwhile
>>>>> set_rx_mode() doesn't seem to depend on any of those and just does the
>>>>> right thing.
>>>>>
>>>>> For what I was trying to do ioctl() was a really easy way out for both
>>>>> kernel and user space implementation, so I gave is shot.
>>>>>
>>>>> -vlad
>>>>>
>>>>
>>>> Don't we already support this with
>>>
>>> The whole point is that these multiple-unicast-address configuration
>>> facilities are inaccessible without FDB, and there is no reason
>>> whatsoever for that.
>>
>> Yes I see now sorry I was behind the thread.
>>
>> We could just use the fdb hooks and add a dflt routine to handle the
>> case where the netdev doesn't provide a specific ndo_op for us. This
>> boils down to calling set_rx_mode anyways through this call chain,
>>
>>     ndo_fdb_add
>>     dev_uc_add_excl
>>         __dev_set_rx_mode
>>             ndo_set_rx_mode
>>
>> As long as folks don't think this is too much of an abuse of the fdb
>> hooks. Here's an example I only compile tested this so take it as
>> an example only. It probably needs some cleanups and the dump routine
>> needs some thought not sure you want to dump all MACs always.
>
> I thought about doing, but this becomes broken on the drivers that
> support ndo_fdb_* functions.  Those drivers mainly require additional
> configuration that is not necessary for dev_uc_add_excl() to work.
>
> For example, ixgbe and melanox requires VF to be on, qlogic needs a
> module parameter, macvlan has to be in passthrough.  However,
> ndo_set_rx_mode() in most cases doesn't care about those settings and
> just works.
>

The ixgbe piece could just use the dflt routines I put below. The
SR-IOV check is only there because when I wrote that I didn't consider
adding additional addresses without VFs. This was a poor example.

The mlx4 card is checking if the device is a slave or master before
adding/removing addresses. My guess is this is the same type of check
as ixgbe and would work just fine without it.

I'm not sure macvlan supports unicast hw addresses either way but
there is no reason we couldn't. The idea was we would come back and
write it later. Like many things I haven't got to it yet.

Calling set_rx_mode() on qlcnic doesn't look to help you at all either
it appears to ignore the uc_list either way.

is that all the callers? looks like it.

> So fdb will not always work even if the driver has proper support for
> IFF_UNICAST_FLT.
>

The fdb could work if we fix the drivers it seems to me most
the cases where it wouldn't work are just lack of foresight or the code
isn't there yet. I think it might be valuable to have a single API for
both use cases.

.John

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