Linux Ethernet Bridging - Apr 2022 - [Bridge] [PATCH net-next v2 0/8] net: bridge: add flush filtering support

On Mon, 11 Apr 2022 23:34:23 +0300 Nikolay Aleksandrov
wrote:
> On 11/04/2022 22:49, Jakub Kicinski wrote:
> >> all great points. My only reason to explore RTM_DELNEIGH is to see
if we
> >> can find a recipe to support similar bulk deletes of other objects
> >> handled via rtm msgs in the future. Plus, it allows you to
maintain
> >> symmetry between flush requests and object delete notification msg
types.
> >>
> >> Lets see if there are other opinions.  
> > 
> > I'd vote for reusing RTM_DELNEIGH, but that's purely based on
> 
> OK, I'll look into the delneigh solution. Note that for backwards
compatibility
> we won't be able to return proper error because rtnl_fdb_del will be
called without
> a mac address, so for old kernels with new iproute2 fdb flush will return
"invalid
> address" as an error.
If only we had policy dump for rtnl :) Another todo item, I guess.
> > intuition, I don't know this code. I'd also lean towards core
> > creating struct net_bridge_fdb_flush_desc rather than piping
> > raw netlink attrs thru. Lastly feels like fdb ops should find   
> 
> I don't think the struct can really be centralized, at least for the
> bridge case it contains private fields which parsed attributes get mapped
to,
> specifically the ndm flags and state, and their maps are all mapped into
> bridge-private flags. Or did you mean pass the raw attribute vals through a
> struct instead of a nlattr table?
Yup, basically the policy is defined in the core, so the types are
known. We can extract the fields from the message there, even if 
the exact meaning of the fields gets established in the callback.

BTW setting NLA_REJECT policy is not required, NLA_REJECT is 0 so 
it will be set automatically per C standard.
> > a new home rather than ndos, but that's largely unrelated..  
> 
> I like separating the ops idea. I'll add that to my bridge todo list.
:)
> 
> Thanks,
>  Nik
>

On 11/04/2022 23:48, Jakub Kicinski wrote:
> On Mon, 11 Apr 2022 23:34:23 +0300 Nikolay Aleksandrov wrote:
>> On 11/04/2022 22:49, Jakub Kicinski wrote:
>>>> all great points. My only reason to explore RTM_DELNEIGH is to
see if we
>>>> can find a recipe to support similar bulk deletes of other
objects
>>>> handled via rtm msgs in the future. Plus, it allows you to
maintain
>>>> symmetry between flush requests and object delete notification
msg types.
>>>>
>>>> Lets see if there are other opinions.  
>>>
>>> I'd vote for reusing RTM_DELNEIGH, but that's purely based
on
>>
>> OK, I'll look into the delneigh solution. Note that for backwards
compatibility
>> we won't be able to return proper error because rtnl_fdb_del will
be called without
>> a mac address, so for old kernels with new iproute2 fdb flush will
return "invalid
>> address" as an error.
> 
> If only we had policy dump for rtnl :) Another todo item, I guess.
> 
:)
>>> intuition, I don't know this code. I'd also lean towards
core
>>> creating struct net_bridge_fdb_flush_desc rather than piping
>>> raw netlink attrs thru. Lastly feels like fdb ops should find   
>>
>> I don't think the struct can really be centralized, at least for
the
>> bridge case it contains private fields which parsed attributes get
mapped to,
>> specifically the ndm flags and state, and their maps are all mapped
into
>> bridge-private flags. Or did you mean pass the raw attribute vals
through a
>> struct instead of a nlattr table?
> 
> Yup, basically the policy is defined in the core, so the types are
> known. We can extract the fields from the message there, even if 
> the exact meaning of the fields gets established in the callback.
> 
That sounds nice, but there are a few catches, f.e. some ndo_fdb implementations
check if attributes were set, i.e. they can also interpret 0, so it will require
additional state (either special value, bitfield or some other way of telling
them
it was actually present but 0).
Anyway I think that is orthogonal to adding the flush support, it's a nice
cleanup but can
be done separately because it will have to be done for all ndo_fdb callbacks and
I
suspect the change will grow considerably.
OTOH the flush implementation via delneigh doesn't require a new ndo_fdb
call way,
would you mind if I finish that up without the struct conversion?
> BTW setting NLA_REJECT policy is not required, NLA_REJECT is 0 so 
> it will be set automatically per C standard.
> 
Indeed - habits, I'll drop it. :)
>>> a new home rather than ndos, but that's largely unrelated..  
>>
>> I like separating the ops idea. I'll add that to my bridge todo
list. :)
>>
>> Thanks,
>>  Nik
>>
>