| 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
| ||
|
Message-ID: <Z17MpOdRsUjBt4Hi@shredder>
Date: Sun, 15 Dec 2024 14:33:40 +0200
From: Ido Schimmel <idosch@...sch.org>
To: Radu Rendec <rrendec@...hat.com>
Cc: Nikolay Aleksandrov <razor@...ckwall.org>,
Roopa Prabhu <roopa@...dia.com>, bridge@...ts.linux.dev,
netdev@...r.kernel.org, Simon Horman <horms@...nel.org>,
Paolo Abeni <pabeni@...hat.com>, Jakub Kicinski <kuba@...nel.org>,
Eric Dumazet <edumazet@...gle.com>,
"David S. Miller" <davem@...emloft.net>
Subject: Re: [PATCH net-next] net/bridge: Add skb drop reasons to the most
common drop points
On Fri, Dec 13, 2024 at 03:44:49PM -0500, Radu Rendec wrote:
> On Thu, 2024-12-12 at 18:14 +0200, Ido Schimmel wrote:
> > On Sun, Dec 08, 2024 at 05:18:05PM -0500, Radu Rendec wrote:
> > > The bridge input code may drop frames for various reasons and at various
> > > points in the ingress handling logic. Currently kfree_skb() is used
> > > everywhere, and therefore no drop reason is specified. Add drop reasons
> > > to the most common drop points.
> > >
> > > The purpose of this patch is to address the most common drop points on
> > > the bridge ingress path. It does not exhaustively add drop reasons to
> > > the entire bridge code. The intention here is to incrementally add drop
> > > reasons to the rest of the bridge code in follow up patches.
> > >
> > > Most of the skb drop points that are addressed in this patch can be
> > > easily tested by sending crafted packets. The diagram below shows a
> > > simple test configuration, and some examples using `packit`(*) are
> > > also included. The bridge is set up with STP disabled.
> > > (*) https://github.com/resurrecting-open-source-projects/packit
> > >
> > > The following changes were *not* tested:
> > > * SKB_DROP_REASON_BRIDGE_NO_EGRESS_PORT in br_multicast_flood(). I could
> > > not find an easy way to make a crafted packet get there.
> > > * SKB_DROP_REASON_BRIDGE_INGRESS_PORT_NFWD in br_handle_frame_finish()
> > > when the port state is BR_STATE_DISABLED, because in that case the
> > > frame is already dropped in the switch/case block at the end of
> > > br_handle_frame().
> > >
> > > +---+---+
> > > | br0 |
> > > +---+---+
> > > |
> > > +---+---+ veth pair +-------+
> > > | veth0 +-------------+ xeth0 |
> > > +-------+ +-------+
> > >
> > > SKB_DROP_REASON_MAC_INVALID_SOURCE - br_handle_frame()
> > > packit -t UDP -s 192.168.0.1 -d 192.168.0.2 -S 8000 -D 8000 \
> > > -e 01:22:33:44:55:66 -E aa:bb:cc:dd:ee:ff -c 1 \
> > > -p '0x de ad be ef' -i xeth0
> > >
> > > SKB_DROP_REASON_MAC_IEEE_MAC_CONTROL - br_handle_frame()
> > > packit -t UDP -s 192.168.0.1 -d 192.168.0.2 -S 8000 -D 8000 \
> > > -e 02:22:33:44:55:66 -E 01:80:c2:00:00:01 -c 1 \
> > > -p '0x de ad be ef' -i xeth0
> > >
> > > SKB_DROP_REASON_BRIDGE_INGRESS_PORT_NFWD - br_handle_frame()
> > > bridge link set dev veth0 state 0 # disabled
> > > packit -t UDP -s 192.168.0.1 -d 192.168.0.2 -S 8000 -D 8000 \
> > > -e 02:22:33:44:55:66 -E aa:bb:cc:dd:ee:ff -c 1 \
> > > -p '0x de ad be ef' -i xeth0
> > >
> > > SKB_DROP_REASON_BRIDGE_INGRESS_PORT_NFWD - br_handle_frame_finish()
> > > bridge link set dev veth0 state 2 # learning
> > > packit -t UDP -s 192.168.0.1 -d 192.168.0.2 -S 8000 -D 8000 \
> > > -e 02:22:33:44:55:66 -E aa:bb:cc:dd:ee:ff -c 1 \
> > > -p '0x de ad be ef' -i xeth0
> > >
> > > SKB_DROP_REASON_BRIDGE_NO_EGRESS_PORT - br_flood()
> > > packit -t UDP -s 192.168.0.1 -d 192.168.0.2 -S 8000 -D 8000 \
> > > -e 02:22:33:44:55:66 -E aa:bb:cc:dd:ee:ff -c 1 \
> > > -p '0x de ad be ef' -i xeth0
> > >
> > > Signed-off-by: Radu Rendec <rrendec@...hat.com>
> > > ---
> > > include/net/dropreason-core.h | 18 ++++++++++++++++++
> > > net/bridge/br_forward.c | 4 ++--
> > > net/bridge/br_input.c | 24 +++++++++++++++---------
> > > 3 files changed, 35 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/include/net/dropreason-core.h b/include/net/dropreason-core.h
> > > index c29282fabae6..1f2ae5b387c1 100644
> > > --- a/include/net/dropreason-core.h
> > > +++ b/include/net/dropreason-core.h
> > > @@ -108,6 +108,9 @@
> > > FN(TUNNEL_TXINFO) \
> > > FN(LOCAL_MAC) \
> > > FN(ARP_PVLAN_DISABLE) \
> > > + FN(MAC_IEEE_MAC_CONTROL) \
> > > + FN(BRIDGE_INGRESS_PORT_NFWD) \
> > > + FN(BRIDGE_NO_EGRESS_PORT) \
> > > FNe(MAX)
> > >
> > > /**
> > > @@ -502,6 +505,21 @@ enum skb_drop_reason {
> > > * enabled.
> > > */
> > > SKB_DROP_REASON_ARP_PVLAN_DISABLE,
> > > + /**
> > > + * @SKB_DROP_REASON_MAC_IEEE_MAC_CONTROL: the destination MAC address
> > > + * is an IEEE MAC Control address.
> > > + */
> >
> > IMO, dropping pause frames is not among "the most common drop points".
> > Are you planning on reusing this reason in other modules? If not, then I
> > prefer removing it. My understanding is that we should not try to
> > document every obscure drop with these reasons.
>
> Fair enough. I don't have an immediate plan to reuse this reason, and
> to be honest, I'm not that familiar with the networking stack to be
> able to tell off hand if it's likely to be useful elsewhere.
>
> Would you prefer to stick to not specifying a drop reason at all at
> that particular drop point, or to reuse an existing reason? Two
> existing reasons that could be used (although they are not entirely
> accurate) are:
> SKB_DROP_REASON_UNHANDLED_PROTO
> SKB_DROP_REASON_MAC_INVALID_SOURCE
Both aren't really applicable in this case and I doubt users are hitting
this drop point in practice, but I feel like I don't have a good
argument against adding 'SKB_DROP_REASON_MAC_IEEE_MAC_CONTROL', so maybe
just keep it ^o^
Powered by blists - more mailing lists