On 2022-08-27 15:17, Ido Schimmel wrote:
On Sat, Aug 27, 2022 at 02:30:25PM +0300, Nikolay Aleksandrov wrote:
On 26/08/2022 14:45, Hans Schultz wrote: Please add the blackhole flag in a separate patch.
+1
[...]
@@ -185,6 +196,9 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb if (test_bit(BR_FDB_LOCAL, &dst->flags)) return br_pass_frame_up(skb);
if (test_bit(BR_FDB_BLACKHOLE, &dst->flags))goto drop;Not happy about adding a new test in arguably the most used fast-path, but I don't see a better way to do blackhole right now. Could you please make it an unlikely() ?
I guess the blackhole flag will be allowed for user-space to set at some point, why not do it from the start?
Actually adding a BR_FDB_LOCAL and BR_FDB_BLACKHOLE would be a conflict above - the packet will be received. So you should move the blackhole check above the BR_FDB_LOCAL one if user-space is allowed to set it to any entry.
Agree about unlikely() and making it writeable from user space from the start. This flag is different from the "locked" flag that should only be ever set by the kernel.
Regarding BR_FDB_LOCAL, I think BR_FDB_BLACKHOLE should only be allowed with BR_FDB_LOCAL as these entries are similar in the following ways:
- It doesn't make sense to associate a blackhole entry with a specific
port. The packet will never be forwarded to this port, but dropped by the bridge. This means user space will add them on the bridge itself:
# bridge fdb add 00:11:22:33:44:55 dev br0 self local blackhole
- If you agree that these entries should not be associated with a
specific port, then it also does not make sense to subject them to ageing and roaming, just like existing local/permanent entries.
The above allows us to push the new check under the BR_FDB_LOCAL check:
diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c index 68b3e850bcb9..4357445529a5 100644 --- a/net/bridge/br_input.c +++ b/net/bridge/br_input.c @@ -182,8 +182,11 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb if (dst) { unsigned long now = jiffies;
if (test_bit(BR_FDB_LOCAL, &dst->flags))
if (test_bit(BR_FDB_LOCAL, &dst->flags)) {if (unlikely(test_bit(BR_FDB_BLACKHOLE,&dst->flags)))
goto drop; return br_pass_frame_up(skb);} if (now != dst->used) dst->used = now;
It shall be so as suggested. :-)