* [PATCH] net: macsec: fix double free of percpu stats
@ 2023-06-13 19:22 Fedor Pchelkin
2023-06-14 3:01 ` Jakub Kicinski
2023-06-15 10:50 ` patchwork-bot+netdevbpf
0 siblings, 2 replies; 9+ messages in thread
From: Fedor Pchelkin @ 2023-06-13 19:22 UTC (permalink / raw)
To: David S. Miller
Cc: Fedor Pchelkin, Sabrina Dubroca, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Raed Salem, Lior Nahmanson, Saeed Mahameed,
Hannes Frederic Sowa, netdev, linux-kernel, Alexey Khoroshilov,
lvc-project
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] net: macsec: fix double free of percpu stats
2023-06-13 19:22 [PATCH] net: macsec: fix double free of percpu stats Fedor Pchelkin
@ 2023-06-14 3:01 ` Jakub Kicinski
2023-06-14 12:26 ` Sabrina Dubroca
2023-06-15 10:50 ` patchwork-bot+netdevbpf
1 sibling, 1 reply; 9+ messages in thread
From: Jakub Kicinski @ 2023-06-14 3:01 UTC (permalink / raw)
To: Fedor Pchelkin
Cc: David S. Miller, Sabrina Dubroca, Eric Dumazet, Paolo Abeni,
Raed Salem, Lior Nahmanson, Saeed Mahameed, Hannes Frederic Sowa,
netdev, linux-kernel, Alexey Khoroshilov, lvc-project
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] net: macsec: fix double free of percpu stats
2023-06-14 3:01 ` Jakub Kicinski
@ 2023-06-14 12:26 ` Sabrina Dubroca
2023-06-14 16:01 ` Jakub Kicinski
0 siblings, 1 reply; 9+ messages in thread
From: Sabrina Dubroca @ 2023-06-14 12:26 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Fedor Pchelkin, David S. Miller, Eric Dumazet, Paolo Abeni,
Raed Salem, Lior Nahmanson, Saeed Mahameed, Hannes Frederic Sowa,
netdev, linux-kernel, Alexey Khoroshilov, lvc-project
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] net: macsec: fix double free of percpu stats
2023-06-14 12:26 ` Sabrina Dubroca
@ 2023-06-14 16:01 ` Jakub Kicinski
2023-06-14 20:17 ` Fedor Pchelkin
0 siblings, 1 reply; 9+ messages in thread
From: Jakub Kicinski @ 2023-06-14 16:01 UTC (permalink / raw)
To: Sabrina Dubroca
Cc: Fedor Pchelkin, David S. Miller, Eric Dumazet, Paolo Abeni,
Raed Salem, Lior Nahmanson, Saeed Mahameed, Hannes Frederic Sowa,
netdev, linux-kernel, Alexey Khoroshilov, lvc-project
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] net: macsec: fix double free of percpu stats
2023-06-14 16:01 ` Jakub Kicinski
@ 2023-06-14 20:17 ` Fedor Pchelkin
2023-06-14 21:15 ` Sabrina Dubroca
0 siblings, 1 reply; 9+ messages in thread
From: Fedor Pchelkin @ 2023-06-14 20:17 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Sabrina Dubroca, David S. Miller, Eric Dumazet, Paolo Abeni,
Raed Salem, Lior Nahmanson, Saeed Mahameed, Hannes Frederic Sowa,
netdev, linux-kernel, Alexey Khoroshilov, lvc-project
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] net: macsec: fix double free of percpu stats
2023-06-14 20:17 ` Fedor Pchelkin
@ 2023-06-14 21:15 ` Sabrina Dubroca
2023-06-15 6:02 ` Jakub Kicinski
0 siblings, 1 reply; 9+ messages in thread
From: Sabrina Dubroca @ 2023-06-14 21:15 UTC (permalink / raw)
To: Fedor Pchelkin
Cc: Jakub Kicinski, David S. Miller, Eric Dumazet, Paolo Abeni,
Raed Salem, Lior Nahmanson, Saeed Mahameed, Hannes Frederic Sowa,
netdev, linux-kernel, Alexey Khoroshilov, lvc-project
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] net: macsec: fix double free of percpu stats
2023-06-14 21:15 ` Sabrina Dubroca
@ 2023-06-15 6:02 ` Jakub Kicinski
2023-06-15 7:07 ` Sabrina Dubroca
0 siblings, 1 reply; 9+ messages in thread
From: Jakub Kicinski @ 2023-06-15 6:02 UTC (permalink / raw)
To: Sabrina Dubroca
Cc: Fedor Pchelkin, David S. Miller, Eric Dumazet, Paolo Abeni,
Raed Salem, Lior Nahmanson, Saeed Mahameed, Hannes Frederic Sowa,
netdev, linux-kernel, Alexey Khoroshilov, lvc-project
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?
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] net: macsec: fix double free of percpu stats
2023-06-15 6:02 ` Jakub Kicinski
@ 2023-06-15 7:07 ` Sabrina Dubroca
0 siblings, 0 replies; 9+ messages in thread
From: Sabrina Dubroca @ 2023-06-15 7:07 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Fedor Pchelkin, David S. Miller, Eric Dumazet, Paolo Abeni,
Raed Salem, Lior Nahmanson, Saeed Mahameed, Hannes Frederic Sowa,
netdev, linux-kernel, Alexey Khoroshilov, lvc-project
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] net: macsec: fix double free of percpu stats
2023-06-13 19:22 [PATCH] net: macsec: fix double free of percpu stats Fedor Pchelkin
2023-06-14 3:01 ` Jakub Kicinski
@ 2023-06-15 10:50 ` patchwork-bot+netdevbpf
1 sibling, 0 replies; 9+ messages in thread
From: patchwork-bot+netdevbpf @ 2023-06-15 10:50 UTC (permalink / raw)
To: Fedor Pchelkin
Cc: davem, sd, edumazet, kuba, pabeni, raeds, liorna, saeedm, hannes,
netdev, linux-kernel, khoroshilov, lvc-project
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
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2023-06-15 10:50 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-06-13 19:22 [PATCH] net: macsec: fix double free of percpu stats Fedor Pchelkin
2023-06-14 3:01 ` Jakub Kicinski
2023-06-14 12:26 ` Sabrina Dubroca
2023-06-14 16:01 ` Jakub Kicinski
2023-06-14 20:17 ` Fedor Pchelkin
2023-06-14 21:15 ` Sabrina Dubroca
2023-06-15 6:02 ` Jakub Kicinski
2023-06-15 7:07 ` Sabrina Dubroca
2023-06-15 10:50 ` patchwork-bot+netdevbpf
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).