Re: [ldv-project] [net] libertas: potential race condition
On Tue, Jun 14, 2016 at 05:16:11PM +0400, Pavel Andrianov wrote: > 08.06.2016 02:51, James Cameron пишет: > >On Tue, Jun 07, 2016 at 09:39:55AM -0500, Dan Williams wrote: > >>On Tue, 2016-06-07 at 13:30 +0400, Pavel Andrianov wrote: > >>>Hi! > >>> > >>>There is a potential race condition in > >>>drivers/net/wireless/libertas/libertas.ko. > >>>In the function lbs_hard_start_xmit(..), line 159, a socket buffer > >>>is > >>>written to priv->current_skb with a spin_lock protection. > >>>In the function lbs_mac_event_disconnected(..), lines 50-51, the > >>>field > >>>current_skb is cleaned. There is no protection used. The > >>>corresponding > >>>handlers are activated at the same time in lbs_start_card(..) and > >>>then > >>>may be executed simultaneously. Note, there are two structures > >>>lbs_netdev_ops and mesh_netdev_ops, which have the target handler > >>>lbs_hard_start_xmit. > >>>Is it a real race or I have missed something? > >>Yeah, it looks like it should be grabbing priv->driver_lock before > >>clearing priv->currenttxskb in lbs_mac_event_disconnected(). Care to > >>submit a patch after testing? Do you have any of that hardware? > >I've hardware, with serial console. > > > >Can test any patch, on USB (8388) or SDIO (8686). > > > Hi! > > I've prepare the patch for this issue. Could you test it? > > Thank you. Tested on OLPC XO-1 (usb8388) and XO-1.5 (sd8686) with v4.7-rc3. Confirmed that lbs_mac_event_disconnected is being called on the station when hostapd on access point is given SIGHUP. Longer duration test was; - SSH to station and run "top -d 0.2", - send SIGHUP every six seconds, for 300 cycles, You may add my; Tested-by: James Cameron-- James Cameron http://quozl.netrek.org/
Re: [ldv-project] [net] libertas: potential race condition
On Tue, Jun 14, 2016 at 05:16:11PM +0400, Pavel Andrianov wrote: > 08.06.2016 02:51, James Cameron пишет: > >On Tue, Jun 07, 2016 at 09:39:55AM -0500, Dan Williams wrote: > >>On Tue, 2016-06-07 at 13:30 +0400, Pavel Andrianov wrote: > >>>Hi! > >>> > >>>There is a potential race condition in > >>>drivers/net/wireless/libertas/libertas.ko. > >>>In the function lbs_hard_start_xmit(..), line 159, a socket buffer > >>>is > >>>written to priv->current_skb with a spin_lock protection. > >>>In the function lbs_mac_event_disconnected(..), lines 50-51, the > >>>field > >>>current_skb is cleaned. There is no protection used. The > >>>corresponding > >>>handlers are activated at the same time in lbs_start_card(..) and > >>>then > >>>may be executed simultaneously. Note, there are two structures > >>>lbs_netdev_ops and mesh_netdev_ops, which have the target handler > >>>lbs_hard_start_xmit. > >>>Is it a real race or I have missed something? > >>Yeah, it looks like it should be grabbing priv->driver_lock before > >>clearing priv->currenttxskb in lbs_mac_event_disconnected(). Care to > >>submit a patch after testing? Do you have any of that hardware? > >I've hardware, with serial console. > > > >Can test any patch, on USB (8388) or SDIO (8686). > > > Hi! > > I've prepare the patch for this issue. Could you test it? > > Thank you. Tested on OLPC XO-1 (usb8388) and XO-1.5 (sd8686) with v4.7-rc3. Confirmed that lbs_mac_event_disconnected is being called on the station when hostapd on access point is given SIGHUP. Longer duration test was; - SSH to station and run "top -d 0.2", - send SIGHUP every six seconds, for 300 cycles, You may add my; Tested-by: James Cameron -- James Cameron http://quozl.netrek.org/
Re: [ldv-project] [net] libertas: potential race condition
08.06.2016 02:51, James Cameron пишет: On Tue, Jun 07, 2016 at 09:39:55AM -0500, Dan Williams wrote: On Tue, 2016-06-07 at 13:30 +0400, Pavel Andrianov wrote: Hi! There is a potential race condition in drivers/net/wireless/libertas/libertas.ko. In the function lbs_hard_start_xmit(..), line 159, a socket buffer is written to priv->current_skb with a spin_lock protection. In the function lbs_mac_event_disconnected(..), lines 50-51, the field current_skb is cleaned. There is no protection used. The corresponding handlers are activated at the same time in lbs_start_card(..) and then may be executed simultaneously. Note, there are two structures lbs_netdev_ops and mesh_netdev_ops, which have the target handler lbs_hard_start_xmit. Is it a real race or I have missed something? Yeah, it looks like it should be grabbing priv->driver_lock before clearing priv->currenttxskb in lbs_mac_event_disconnected(). Care to submit a patch after testing? Do you have any of that hardware? I've hardware, with serial console. Can test any patch, on USB (8388) or SDIO (8686). Hi! I've prepare the patch for this issue. Could you test it? Thank you. -- Pavel Andrianov Linux Verification Center, ISPRAS web: http://linuxtesting.org e-mail: andria...@ispras.ru >From b1a9e157ccdf8650da93844fd2dbdb6fca509b59 Mon Sep 17 00:00:00 2001 From: PavelDate: Tue, 14 Jun 2016 16:59:00 +0400 Subject: [PATCH] libertas: Add spinlock to avoid race condition lbs_mac_event_disconnected may free priv->currenttxskb while lbs_hard_start_xmit accesses to it. The patch adds a spinlock for mutual exclusion. Signed-off-by: Pavel --- drivers/net/wireless/marvell/libertas/cmdresp.c | 4 1 file changed, 4 insertions(+) diff --git a/drivers/net/wireless/marvell/libertas/cmdresp.c b/drivers/net/wireless/marvell/libertas/cmdresp.c index c95bf6d..c67ae07 100644 --- a/drivers/net/wireless/marvell/libertas/cmdresp.c +++ b/drivers/net/wireless/marvell/libertas/cmdresp.c @@ -27,6 +27,8 @@ void lbs_mac_event_disconnected(struct lbs_private *priv, bool locally_generated) { + unsigned long flags; + if (priv->connect_status != LBS_CONNECTED) return; @@ -46,9 +48,11 @@ void lbs_mac_event_disconnected(struct lbs_private *priv, netif_carrier_off(priv->dev); /* Free Tx and Rx packets */ + spin_lock_irqsave(>driver_lock, flags); kfree_skb(priv->currenttxskb); priv->currenttxskb = NULL; priv->tx_pending_len = 0; + spin_unlock_irqrestore(>driver_lock, flags); priv->connect_status = LBS_DISCONNECTED; -- 1.7.11.7
Re: [ldv-project] [net] libertas: potential race condition
08.06.2016 02:51, James Cameron пишет: On Tue, Jun 07, 2016 at 09:39:55AM -0500, Dan Williams wrote: On Tue, 2016-06-07 at 13:30 +0400, Pavel Andrianov wrote: Hi! There is a potential race condition in drivers/net/wireless/libertas/libertas.ko. In the function lbs_hard_start_xmit(..), line 159, a socket buffer is written to priv->current_skb with a spin_lock protection. In the function lbs_mac_event_disconnected(..), lines 50-51, the field current_skb is cleaned. There is no protection used. The corresponding handlers are activated at the same time in lbs_start_card(..) and then may be executed simultaneously. Note, there are two structures lbs_netdev_ops and mesh_netdev_ops, which have the target handler lbs_hard_start_xmit. Is it a real race or I have missed something? Yeah, it looks like it should be grabbing priv->driver_lock before clearing priv->currenttxskb in lbs_mac_event_disconnected(). Care to submit a patch after testing? Do you have any of that hardware? I've hardware, with serial console. Can test any patch, on USB (8388) or SDIO (8686). Hi! I've prepare the patch for this issue. Could you test it? Thank you. -- Pavel Andrianov Linux Verification Center, ISPRAS web: http://linuxtesting.org e-mail: andria...@ispras.ru >From b1a9e157ccdf8650da93844fd2dbdb6fca509b59 Mon Sep 17 00:00:00 2001 From: Pavel Date: Tue, 14 Jun 2016 16:59:00 +0400 Subject: [PATCH] libertas: Add spinlock to avoid race condition lbs_mac_event_disconnected may free priv->currenttxskb while lbs_hard_start_xmit accesses to it. The patch adds a spinlock for mutual exclusion. Signed-off-by: Pavel --- drivers/net/wireless/marvell/libertas/cmdresp.c | 4 1 file changed, 4 insertions(+) diff --git a/drivers/net/wireless/marvell/libertas/cmdresp.c b/drivers/net/wireless/marvell/libertas/cmdresp.c index c95bf6d..c67ae07 100644 --- a/drivers/net/wireless/marvell/libertas/cmdresp.c +++ b/drivers/net/wireless/marvell/libertas/cmdresp.c @@ -27,6 +27,8 @@ void lbs_mac_event_disconnected(struct lbs_private *priv, bool locally_generated) { + unsigned long flags; + if (priv->connect_status != LBS_CONNECTED) return; @@ -46,9 +48,11 @@ void lbs_mac_event_disconnected(struct lbs_private *priv, netif_carrier_off(priv->dev); /* Free Tx and Rx packets */ + spin_lock_irqsave(>driver_lock, flags); kfree_skb(priv->currenttxskb); priv->currenttxskb = NULL; priv->tx_pending_len = 0; + spin_unlock_irqrestore(>driver_lock, flags); priv->connect_status = LBS_DISCONNECTED; -- 1.7.11.7
Re: [ldv-project] [net] libertas: potential race condition
On Tue, Jun 07, 2016 at 09:39:55AM -0500, Dan Williams wrote: > On Tue, 2016-06-07 at 13:30 +0400, Pavel Andrianov wrote: > > Hi! > > > > There is a potential race condition in > > drivers/net/wireless/libertas/libertas.ko. > > In the function lbs_hard_start_xmit(..), line 159, a socket buffer > > is > > written to priv->current_skb with a spin_lock protection. > > In the function lbs_mac_event_disconnected(..), lines 50-51, the > > field > > current_skb is cleaned. There is no protection used. The > > corresponding > > handlers are activated at the same time in lbs_start_card(..) and > > then > > may be executed simultaneously. Note, there are two structures > > lbs_netdev_ops and mesh_netdev_ops, which have the target handler > > lbs_hard_start_xmit. > > Is it a real race or I have missed something? > > Yeah, it looks like it should be grabbing priv->driver_lock before > clearing priv->currenttxskb in lbs_mac_event_disconnected(). Care to > submit a patch after testing? Do you have any of that hardware? I've hardware, with serial console. Can test any patch, on USB (8388) or SDIO (8686). -- James Cameron http://quozl.netrek.org/
Re: [ldv-project] [net] libertas: potential race condition
On Tue, Jun 07, 2016 at 09:39:55AM -0500, Dan Williams wrote: > On Tue, 2016-06-07 at 13:30 +0400, Pavel Andrianov wrote: > > Hi! > > > > There is a potential race condition in > > drivers/net/wireless/libertas/libertas.ko. > > In the function lbs_hard_start_xmit(..), line 159, a socket buffer > > is > > written to priv->current_skb with a spin_lock protection. > > In the function lbs_mac_event_disconnected(..), lines 50-51, the > > field > > current_skb is cleaned. There is no protection used. The > > corresponding > > handlers are activated at the same time in lbs_start_card(..) and > > then > > may be executed simultaneously. Note, there are two structures > > lbs_netdev_ops and mesh_netdev_ops, which have the target handler > > lbs_hard_start_xmit. > > Is it a real race or I have missed something? > > Yeah, it looks like it should be grabbing priv->driver_lock before > clearing priv->currenttxskb in lbs_mac_event_disconnected(). Care to > submit a patch after testing? Do you have any of that hardware? I've hardware, with serial console. Can test any patch, on USB (8388) or SDIO (8686). -- James Cameron http://quozl.netrek.org/
Re: [ldv-project] [net] libertas: potential race condition
07.06.2016 18:39, Dan Williams пишет: On Tue, 2016-06-07 at 13:30 +0400, Pavel Andrianov wrote: Hi! There is a potential race condition in drivers/net/wireless/libertas/libertas.ko. In the function lbs_hard_start_xmit(..), line 159, a socket buffer is written to priv->current_skb with a spin_lock protection. In the function lbs_mac_event_disconnected(..), lines 50-51, the field current_skb is cleaned. There is no protection used. The corresponding handlers are activated at the same time in lbs_start_card(..) and then may be executed simultaneously. Note, there are two structures lbs_netdev_ops and mesh_netdev_ops, which have the target handler lbs_hard_start_xmit. Is it a real race or I have missed something? Yeah, it looks like it should be grabbing priv->driver_lock before clearing priv->currenttxskb in lbs_mac_event_disconnected(). Care to submit a patch after testing? Do you have any of that hardware? Dan I have no that hardware and I have some doubts about the simple fix, you've suggested. For instance, in lbs_hard_start_xmit the lock is acquired twice and the priv->tx_pending_len can be modified also by lbs_mac_event_disconnected (even if spin_lock will be added to lbs_mac_event_disconnected). Moreover, the function lbs_send_tx_feedback also cleaned priv->currenttxskb, but it happens also without any protection. Thus, the fix has to be more complicated, and I have no ideas about it. -- Pavel Andrianov Linux Verification Center, ISPRAS web: http://linuxtesting.org e-mail: andria...@ispras.ru
Re: [ldv-project] [net] libertas: potential race condition
07.06.2016 18:39, Dan Williams пишет: On Tue, 2016-06-07 at 13:30 +0400, Pavel Andrianov wrote: Hi! There is a potential race condition in drivers/net/wireless/libertas/libertas.ko. In the function lbs_hard_start_xmit(..), line 159, a socket buffer is written to priv->current_skb with a spin_lock protection. In the function lbs_mac_event_disconnected(..), lines 50-51, the field current_skb is cleaned. There is no protection used. The corresponding handlers are activated at the same time in lbs_start_card(..) and then may be executed simultaneously. Note, there are two structures lbs_netdev_ops and mesh_netdev_ops, which have the target handler lbs_hard_start_xmit. Is it a real race or I have missed something? Yeah, it looks like it should be grabbing priv->driver_lock before clearing priv->currenttxskb in lbs_mac_event_disconnected(). Care to submit a patch after testing? Do you have any of that hardware? Dan I have no that hardware and I have some doubts about the simple fix, you've suggested. For instance, in lbs_hard_start_xmit the lock is acquired twice and the priv->tx_pending_len can be modified also by lbs_mac_event_disconnected (even if spin_lock will be added to lbs_mac_event_disconnected). Moreover, the function lbs_send_tx_feedback also cleaned priv->currenttxskb, but it happens also without any protection. Thus, the fix has to be more complicated, and I have no ideas about it. -- Pavel Andrianov Linux Verification Center, ISPRAS web: http://linuxtesting.org e-mail: andria...@ispras.ru
Re: [ldv-project] [net] libertas: potential race condition
On Tue, 2016-06-07 at 13:30 +0400, Pavel Andrianov wrote: > Hi! > > There is a potential race condition in > drivers/net/wireless/libertas/libertas.ko. > In the function lbs_hard_start_xmit(..), line 159, a socket buffer > is > written to priv->current_skb with a spin_lock protection. > In the function lbs_mac_event_disconnected(..), lines 50-51, the > field > current_skb is cleaned. There is no protection used. The > corresponding > handlers are activated at the same time in lbs_start_card(..) and > then > may be executed simultaneously. Note, there are two structures > lbs_netdev_ops and mesh_netdev_ops, which have the target handler > lbs_hard_start_xmit. > Is it a real race or I have missed something? Yeah, it looks like it should be grabbing priv->driver_lock before clearing priv->currenttxskb in lbs_mac_event_disconnected(). Care to submit a patch after testing? Do you have any of that hardware? Dan
Re: [ldv-project] [net] libertas: potential race condition
On Tue, 2016-06-07 at 13:30 +0400, Pavel Andrianov wrote: > Hi! > > There is a potential race condition in > drivers/net/wireless/libertas/libertas.ko. > In the function lbs_hard_start_xmit(..), line 159, a socket buffer > is > written to priv->current_skb with a spin_lock protection. > In the function lbs_mac_event_disconnected(..), lines 50-51, the > field > current_skb is cleaned. There is no protection used. The > corresponding > handlers are activated at the same time in lbs_start_card(..) and > then > may be executed simultaneously. Note, there are two structures > lbs_netdev_ops and mesh_netdev_ops, which have the target handler > lbs_hard_start_xmit. > Is it a real race or I have missed something? Yeah, it looks like it should be grabbing priv->driver_lock before clearing priv->currenttxskb in lbs_mac_event_disconnected(). Care to submit a patch after testing? Do you have any of that hardware? Dan