netdev - Re: [PATCH 1/2] net: dsa: RCU-protect dsa_ptr in struct net

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240913190326.xv5qkxt7b3sjuroz@skbuf>
Date: Fri, 13 Sep 2024 22:03:26 +0300
From: Vladimir Oltean <olteanv@...il.com>
To: "A. Sverdlin" <alexander.sverdlin@...mens.com>
Cc: netdev@...r.kernel.org,
	Ar1nç ÜNAL <arinc.unal@...nc9.com>,
	Daniel Golle <daniel@...rotopia.org>,
	DENG Qingfang <dqfext@...il.com>,
	Sean Wang <sean.wang@...iatek.com>, Andrew Lunn <andrew@...n.ch>,
	Florian Fainelli <f.fainelli@...il.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Matthias Brugger <matthias.bgg@...il.com>,
	AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
	Claudiu Manoil <claudiu.manoil@....com>,
	Alexandre Belloni <alexandre.belloni@...tlin.com>,
	UNGLinuxDriver@...rochip.com,
	Broadcom internal kernel review list <bcm-kernel-feedback-list@...adcom.com>,
	Lorenzo Bianconi <lorenzo@...nel.org>, Felix Fietkau <nbd@....name>,
	Mark Lee <Mark-MC.Lee@...iatek.com>,
	Roopa Prabhu <roopa@...dia.com>,
	Nikolay Aleksandrov <razor@...ckwall.org>,
	linux-mediatek@...ts.infradead.org, bridge@...ts.linux.dev,
	stable@...r.kernel.org
Subject: Re: [PATCH 1/2] net: dsa: RCU-protect dsa_ptr in struct net_device

Hi Alexander,

On Tue, Sep 10, 2024 at 03:03:15PM +0200, A. Sverdlin wrote:
> From: Alexander Sverdlin <alexander.sverdlin@...mens.com>
> 
> There are multiple races of zeroing dsa_ptr in struct net_device (on
> shutdown/remove) against asynchronous dereferences all over the net
> code. Widespread pattern is as follows:
> 
> CPU0					CPU1
> if (netdev_uses_dsa())
> 					dev->dsa_ptr = NULL;
>         dev->dsa_ptr->...
> 
> One of the possible crashes:
> 
> Unable to handle kernel NULL pointer dereference at virtual address 0000000000000010
> CPU: 0 PID: 12 Comm: ksoftirqd/0 Tainted: G O 6.1.99+ #1
> pc : lan9303_rcv
> lr : lan9303_rcv
> Call trace:
>  lan9303_rcv
>  dsa_switch_rcv
>  __netif_receive_skb_list_core
>  netif_receive_skb_list_internal
>  napi_gro_receive
>  fec_enet_rx_napi
>  __napi_poll
>  net_rx_action
> ...
> 
> RCU-protect dsa_ptr and use rcu_dereference() or rtnl_dereference()
> depending on the calling context.
> 
> Rename netdev_uses_dsa() into __netdev_uses_dsa_currently()
> (assumes ether RCU or RTNL lock held) and netdev_uses_dsa_currently()
> variants which better reflect the uselessness of the function's
> return value, which becomes outdated right after the call.
> 
> Fixes: ee534378f005 ("net: dsa: fix panic when DSA master device unbinds on shutdown")
> Cc: stable@...r.kernel.org
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@...mens.com>
> ---
>  drivers/net/dsa/mt7530.c                    |   3 +-
>  drivers/net/dsa/ocelot/felix.c              |   3 +-
>  drivers/net/dsa/qca/qca8k-8xxx.c            |   3 +-
>  drivers/net/ethernet/broadcom/bcmsysport.c  |   8 +-
>  drivers/net/ethernet/mediatek/airoha_eth.c  |   2 +-
>  drivers/net/ethernet/mediatek/mtk_eth_soc.c |  22 +++--
>  drivers/net/ethernet/mediatek/mtk_ppe.c     |  15 ++-
>  include/linux/netdevice.h                   |   2 +-
>  include/net/dsa.h                           |  36 +++++--
>  include/net/dsa_stubs.h                     |   6 +-
>  net/bridge/br_input.c                       |   2 +-
>  net/core/dev.c                              |   3 +-
>  net/core/flow_dissector.c                   |  19 ++--
>  net/dsa/conduit.c                           |  66 ++++++++-----
>  net/dsa/dsa.c                               |  19 ++--
>  net/dsa/port.c                              |   3 +-
>  net/dsa/tag.c                               |   3 +-
>  net/dsa/tag.h                               |  19 ++--
>  net/dsa/tag_8021q.c                         |  10 +-
>  net/dsa/tag_brcm.c                          |   2 +-
>  net/dsa/tag_dsa.c                           |   8 +-
>  net/dsa/tag_qca.c                           |  10 +-
>  net/dsa/tag_sja1105.c                       |  22 +++--
>  net/dsa/user.c                              | 104 +++++++++++---------
>  net/ethernet/eth.c                          |   2 +-
>  25 files changed, 240 insertions(+), 152 deletions(-)

Thank you for the patch, and I would like you to not give up on it, even
if we will go for a different bug fix for 'stable'.

It's just that it makes me a bit uneasy to have this as the bug fix.
"Stable" kernels are supposedly named as such because you're not backporting
such a major usage pattern change from the core down to all switch drivers
and N other Ethernet drivers which you can't even test. The diffstat
here says it all, I believe.

This kind of change, in principle, is okay for an early net-next opening.
I have it in my tree, have made a few changes to it, will run more tests
on the HW I have, and will let you know over the course of the following
weeks when it reaches a stage that I'm also comfortable with.