Inside macsec_add_dev() we free percpu macsec->secy.tx_sc.stats and
macsec->stats on some of the memory allocation failure paths. However, the
net_device is already registered to that moment: in macsec_newlink(), just
before calling macsec_add_dev(). This means that during unregister process
its priv_destructor - macsec_free_netdev() - will be called and will free
the stats again.

Remove freeing percpu stats inside macsec_add_dev() because
macsec_free_netdev() will correctly free the already allocated ones. The
pointers to unallocated stats stay NULL, and free_percpu() treats that
correctly.

Found by Linux Verification Center (linuxtesting.org) with Syzkaller.

Fixes: 0a28bfd4971f ("net/macsec: Add MACsec skb_metadata_dst Tx Data path support")
Fixes: c09440f7dcb3 ("macsec: introduce IEEE 802.1AE driver")
Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru>
---
 drivers/net/macsec.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
index 3427993f94f7..984dfa5d6c11 100644
--- a/drivers/net/macsec.c
+++ b/drivers/net/macsec.c
@@ -3997,17 +3997,15 @@ static int macsec_add_dev(struct net_device *dev, sci_t sci, u8 icv_len)
 		return -ENOMEM;
 
 	secy->tx_sc.stats = netdev_alloc_pcpu_stats(struct pcpu_tx_sc_stats);
-	if (!secy->tx_sc.stats) {
-		free_percpu(macsec->stats);
+	if (!secy->tx_sc.stats)
 		return -ENOMEM;
-	}
 
 	secy->tx_sc.md_dst = metadata_dst_alloc(0, METADATA_MACSEC, GFP_KERNEL);
-	if (!secy->tx_sc.md_dst) {
-		free_percpu(secy->tx_sc.stats);
-		free_percpu(macsec->stats);
+	if (!secy->tx_sc.md_dst)
+		/* macsec and secy percpu stats will be freed when unregistering
+		 * net_device in macsec_free_netdev()
+		 */
 		return -ENOMEM;
-	}
 
 	if (sci == MACSEC_UNDEF_SCI)
 		sci = dev_to_sci(dev, MACSEC_PORT_ES);
-- 
2.34.1

Hello:

This patch was applied to netdev/net.git (main)
by David S. Miller <davem@davemloft.net>:

On Tue, 13 Jun 2023 22:22:20 +0300 you wrote:
> Inside macsec_add_dev() we free percpu macsec->secy.tx_sc.stats and
> macsec->stats on some of the memory allocation failure paths. However, the
> net_device is already registered to that moment: in macsec_newlink(), just
> before calling macsec_add_dev(). This means that during unregister process
> its priv_destructor - macsec_free_netdev() - will be called and will free
> the stats again.
> 
> [...]

Here is the summary with links:
  - net: macsec: fix double free of percpu stats
    https://git.kernel.org/netdev/net/c/0c0cf3db83f8

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

On Tue, 13 Jun 2023 22:22:20 +0300 Fedor Pchelkin wrote:
> Inside macsec_add_dev() we free percpu macsec->secy.tx_sc.stats and
> macsec->stats on some of the memory allocation failure paths. However, the
> net_device is already registered to that moment: in macsec_newlink(), just
> before calling macsec_add_dev(). This means that during unregister process
> its priv_destructor - macsec_free_netdev() - will be called and will free
> the stats again.
> 
> Remove freeing percpu stats inside macsec_add_dev() because
> macsec_free_netdev() will correctly free the already allocated ones. The
> pointers to unallocated stats stay NULL, and free_percpu() treats that
> correctly.

What prevents the device from being opened and used before
macsec_add_dev() has finished? I think we need a fix which 
would move this code before register_netdev(), instead :(
-- 
pw-bot: cr

2023-06-13, 20:01:50 -0700, Jakub Kicinski wrote:
> On Tue, 13 Jun 2023 22:22:20 +0300 Fedor Pchelkin wrote:
> > Inside macsec_add_dev() we free percpu macsec->secy.tx_sc.stats and
> > macsec->stats on some of the memory allocation failure paths. However, the
> > net_device is already registered to that moment: in macsec_newlink(), just
> > before calling macsec_add_dev(). This means that during unregister process
> > its priv_destructor - macsec_free_netdev() - will be called and will free
> > the stats again.
> > 
> > Remove freeing percpu stats inside macsec_add_dev() because
> > macsec_free_netdev() will correctly free the already allocated ones. The
> > pointers to unallocated stats stay NULL, and free_percpu() treats that
> > correctly.
> 
> What prevents the device from being opened and used before
> macsec_add_dev() has finished? I think we need a fix which 
> would move this code before register_netdev(), instead :(

Can the device be opened in parallel? We're under rtnl here.

If we want to move that code, then we'll also have to move the
eth_hw_addr_inherit call that's currently in macsec's ndo_init: in
case the user didn't give an SCI, we have to make it up based on the
device's mac address (dev_to_sci(dev, ...)), whether it's set by the
user or inherited. I can't remember if I had a good reason to put the
inherit in ndo_init.

-- 
Sabrina

On Wed, 14 Jun 2023 14:26:14 +0200 Sabrina Dubroca wrote:
> > What prevents the device from being opened and used before
> > macsec_add_dev() has finished? I think we need a fix which 
> > would move this code before register_netdev(), instead :(  
> 
> Can the device be opened in parallel? We're under rtnl here.
> 
> If we want to move that code, then we'll also have to move the
> eth_hw_addr_inherit call that's currently in macsec's ndo_init: in
> case the user didn't give an SCI, we have to make it up based on the
> device's mac address (dev_to_sci(dev, ...)), whether it's set by the
> user or inherited. I can't remember if I had a good reason to put the
> inherit in ndo_init.

Ah, you're right, this is a link creation path.
-- 
pw-bot: ur

On Wed, Jun 14, 2023 at 09:01:26AM -0700, Jakub Kicinski wrote:
> On Wed, 14 Jun 2023 14:26:14 +0200 Sabrina Dubroca wrote:
> > > What prevents the device from being opened and used before
> > > macsec_add_dev() has finished? I think we need a fix which 
> > > would move this code before register_netdev(), instead :(  
> > 
> > Can the device be opened in parallel? We're under rtnl here.
> > 
> > If we want to move that code, then we'll also have to move the
> > eth_hw_addr_inherit call that's currently in macsec's ndo_init: in
> > case the user didn't give an SCI, we have to make it up based on the
> > device's mac address (dev_to_sci(dev, ...)), whether it's set by the
> > user or inherited. I can't remember if I had a good reason to put the
> > inherit in ndo_init.
> 
> Ah, you're right, this is a link creation path.

My reply probably won't give any new information now but if the code of
macsec_add_dev() and the parts from ndo_init it depends on which Sabrina
mentioned would be moved before registering netdev then the problem will
go away on its own.

Is it worth moving that code if rtnl_lock is held? Maybe it will be more
persistent to initialize the device for as maximum as possible before
calling register_netdevice()? Overall, it all depends on the reasons why
the code was implemented so initially.

2023-06-14, 23:17:14 +0300, Fedor Pchelkin wrote:
> On Wed, Jun 14, 2023 at 09:01:26AM -0700, Jakub Kicinski wrote:
> > On Wed, 14 Jun 2023 14:26:14 +0200 Sabrina Dubroca wrote:
> > > > What prevents the device from being opened and used before
> > > > macsec_add_dev() has finished? I think we need a fix which 
> > > > would move this code before register_netdev(), instead :(  
> > > 
> > > Can the device be opened in parallel? We're under rtnl here.
> > > 
> > > If we want to move that code, then we'll also have to move the
> > > eth_hw_addr_inherit call that's currently in macsec's ndo_init: in
> > > case the user didn't give an SCI, we have to make it up based on the
> > > device's mac address (dev_to_sci(dev, ...)), whether it's set by the
> > > user or inherited. I can't remember if I had a good reason to put the
> > > inherit in ndo_init.
> > 
> > Ah, you're right, this is a link creation path.
> 
> My reply probably won't give any new information now but if the code of
> macsec_add_dev() and the parts from ndo_init it depends on which Sabrina
> mentioned would be moved before registering netdev then the problem will
> go away on its own.
> 
> Is it worth moving that code if rtnl_lock is held? Maybe it will be more
> persistent to initialize the device for as maximum as possible before
> calling register_netdevice()? Overall, it all depends on the reasons why
> the code was implemented so initially.

It's been 7 years... your guess is about as good as mine :/

I wouldn't bother reshuffling the device creation code just to make
the handling of rare failures a bit nicer.

-- 
Sabrina

On Wed, 14 Jun 2023 23:15:03 +0200 Sabrina Dubroca wrote:
> It's been 7 years... your guess is about as good as mine :/
> 
> I wouldn't bother reshuffling the device creation code just to make
> the handling of rare failures a bit nicer.

Would you be willing to venture a review tag?

2023-06-14, 23:02:39 -0700, Jakub Kicinski wrote:
> On Wed, 14 Jun 2023 23:15:03 +0200 Sabrina Dubroca wrote:
> > It's been 7 years... your guess is about as good as mine :/
> > 
> > I wouldn't bother reshuffling the device creation code just to make
> > the handling of rare failures a bit nicer.
> 
> Would you be willing to venture a review tag?

Reviewed-by: Sabrina Dubroca <sd@queasysnail.net>

-- 
Sabrina