diff mbox series

[v2,net,2/8] net: mscc: ocelot: add locking for the port TX timestamp ID

Message ID 20200918010730.2911234-3-olteanv@gmail.com
State New
Headers show
Series [v2,net,1/8] net: mscc: ocelot: fix race condition with TX timestamping | expand

Commit Message

Vladimir Oltean Sept. 18, 2020, 1:07 a.m. UTC
From: Vladimir Oltean <vladimir.oltean@nxp.com>

The ocelot_port->ts_id is used to:
(a) populate skb->cb[0] for matching the TX timestamp in the PTP IRQ
    with an skb.
(b) populate the REW_OP from the injection header of the ongoing skb.
Only then is ocelot_port->ts_id incremented.

This is a problem because, at least theoretically, another timestampable
skb might use the same ocelot_port->ts_id before that is incremented.
Normally all transmit calls are serialized by the netdev transmit
spinlock, but in this case, ocelot_port_add_txtstamp_skb() is also
called by DSA, which has started declaring the NETIF_F_LLTX feature
since commit 2b86cb829976 ("net: dsa: declare lockless TX feature for
slave ports").  So the logic of using and incrementing the timestamp id
should be atomic per port.

The solution is to use the global ocelot_port->ts_id only while
protected by the associated ocelot_port->ts_id_lock. That's where we
populate skb->cb[0]. Note that for ocelot, ocelot_port_add_txtstamp_skb
is called for the actual skb, but for felix, it is called for the skb's
clone. That is something which will also be changed in the future.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Reviewed-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
Changes in v2:
Added an extra explanation about NETIF_F_LLTX in commit message.

 drivers/net/ethernet/mscc/ocelot.c     |  8 +++++++-
 drivers/net/ethernet/mscc/ocelot_net.c |  6 ++----
 include/soc/mscc/ocelot.h              |  1 +
 net/dsa/tag_ocelot.c                   | 11 +++++++----
 4 files changed, 17 insertions(+), 9 deletions(-)

Comments

Florian Fainelli Sept. 18, 2020, 2:37 a.m. UTC | #1
On 9/17/2020 6:07 PM, Vladimir Oltean wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>

> 

> The ocelot_port->ts_id is used to:

> (a) populate skb->cb[0] for matching the TX timestamp in the PTP IRQ

>      with an skb.

> (b) populate the REW_OP from the injection header of the ongoing skb.

> Only then is ocelot_port->ts_id incremented.

> 

> This is a problem because, at least theoretically, another timestampable

> skb might use the same ocelot_port->ts_id before that is incremented.

> Normally all transmit calls are serialized by the netdev transmit

> spinlock, but in this case, ocelot_port_add_txtstamp_skb() is also

> called by DSA, which has started declaring the NETIF_F_LLTX feature

> since commit 2b86cb829976 ("net: dsa: declare lockless TX feature for

> slave ports").  So the logic of using and incrementing the timestamp id

> should be atomic per port.

> 

> The solution is to use the global ocelot_port->ts_id only while

> protected by the associated ocelot_port->ts_id_lock. That's where we

> populate skb->cb[0]. Note that for ocelot, ocelot_port_add_txtstamp_skb

> is called for the actual skb, but for felix, it is called for the skb's

> clone. That is something which will also be changed in the future.

> 

> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

> Reviewed-by: Horatiu Vultur <horatiu.vultur@microchip.com>


Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>

-- 
Florian
Alexandre Belloni Sept. 18, 2020, 3:27 p.m. UTC | #2
On 18/09/2020 04:07:24+0300, Vladimir Oltean wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>

> 

> The ocelot_port->ts_id is used to:

> (a) populate skb->cb[0] for matching the TX timestamp in the PTP IRQ

>     with an skb.

> (b) populate the REW_OP from the injection header of the ongoing skb.

> Only then is ocelot_port->ts_id incremented.

> 

> This is a problem because, at least theoretically, another timestampable

> skb might use the same ocelot_port->ts_id before that is incremented.

> Normally all transmit calls are serialized by the netdev transmit

> spinlock, but in this case, ocelot_port_add_txtstamp_skb() is also

> called by DSA, which has started declaring the NETIF_F_LLTX feature

> since commit 2b86cb829976 ("net: dsa: declare lockless TX feature for

> slave ports").  So the logic of using and incrementing the timestamp id

> should be atomic per port.

> 

> The solution is to use the global ocelot_port->ts_id only while

> protected by the associated ocelot_port->ts_id_lock. That's where we

> populate skb->cb[0]. Note that for ocelot, ocelot_port_add_txtstamp_skb

> is called for the actual skb, but for felix, it is called for the skb's

> clone. That is something which will also be changed in the future.

> 

> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

> Reviewed-by: Horatiu Vultur <horatiu.vultur@microchip.com>


Tested-by: Alexandre Belloni <alexandre.belloni@bootlin.com>

Reviewed-by: Alexandre Belloni <alexandre.belloni@bootlin.com>


> ---

> Changes in v2:

> Added an extra explanation about NETIF_F_LLTX in commit message.

> 

>  drivers/net/ethernet/mscc/ocelot.c     |  8 +++++++-

>  drivers/net/ethernet/mscc/ocelot_net.c |  6 ++----

>  include/soc/mscc/ocelot.h              |  1 +

>  net/dsa/tag_ocelot.c                   | 11 +++++++----

>  4 files changed, 17 insertions(+), 9 deletions(-)

> 

> diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c

> index 5abb7d2b0a9e..83eb7c325061 100644

> --- a/drivers/net/ethernet/mscc/ocelot.c

> +++ b/drivers/net/ethernet/mscc/ocelot.c

> @@ -421,10 +421,15 @@ int ocelot_port_add_txtstamp_skb(struct ocelot_port *ocelot_port,

>  

>  	if (ocelot->ptp && shinfo->tx_flags & SKBTX_HW_TSTAMP &&

>  	    ocelot_port->ptp_cmd == IFH_REW_OP_TWO_STEP_PTP) {

> +		spin_lock(&ocelot_port->ts_id_lock);

> +

>  		shinfo->tx_flags |= SKBTX_IN_PROGRESS;

>  		/* Store timestamp ID in cb[0] of sk_buff */

> -		skb->cb[0] = ocelot_port->ts_id % 4;

> +		skb->cb[0] = ocelot_port->ts_id;

> +		ocelot_port->ts_id = (ocelot_port->ts_id + 1) % 4;

>  		skb_queue_tail(&ocelot_port->tx_skbs, skb);

> +

> +		spin_unlock(&ocelot_port->ts_id_lock);

>  		return 0;

>  	}

>  	return -ENODATA;

> @@ -1300,6 +1305,7 @@ void ocelot_init_port(struct ocelot *ocelot, int port)

>  	struct ocelot_port *ocelot_port = ocelot->ports[port];

>  

>  	skb_queue_head_init(&ocelot_port->tx_skbs);

> +	spin_lock_init(&ocelot_port->ts_id_lock);

>  

>  	/* Basic L2 initialization */

>  

> diff --git a/drivers/net/ethernet/mscc/ocelot_net.c b/drivers/net/ethernet/mscc/ocelot_net.c

> index cacabc23215a..8490e42e9e2d 100644

> --- a/drivers/net/ethernet/mscc/ocelot_net.c

> +++ b/drivers/net/ethernet/mscc/ocelot_net.c

> @@ -349,10 +349,8 @@ static int ocelot_port_xmit(struct sk_buff *skb, struct net_device *dev)

>  

>  	if (ocelot->ptp && shinfo->tx_flags & SKBTX_HW_TSTAMP) {

>  		info.rew_op = ocelot_port->ptp_cmd;

> -		if (ocelot_port->ptp_cmd == IFH_REW_OP_TWO_STEP_PTP) {

> -			info.rew_op |= (ocelot_port->ts_id  % 4) << 3;

> -			ocelot_port->ts_id++;

> -		}

> +		if (ocelot_port->ptp_cmd == IFH_REW_OP_TWO_STEP_PTP)

> +			info.rew_op |= skb->cb[0] << 3;

>  	}

>  

>  	ocelot_gen_ifh(ifh, &info);

> diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h

> index da369b12005f..4521dd602ddc 100644

> --- a/include/soc/mscc/ocelot.h

> +++ b/include/soc/mscc/ocelot.h

> @@ -566,6 +566,7 @@ struct ocelot_port {

>  	u8				ptp_cmd;

>  	struct sk_buff_head		tx_skbs;

>  	u8				ts_id;

> +	spinlock_t			ts_id_lock;

>  

>  	phy_interface_t			phy_mode;

>  

> diff --git a/net/dsa/tag_ocelot.c b/net/dsa/tag_ocelot.c

> index 42f327c06dca..b4fc05cafaa6 100644

> --- a/net/dsa/tag_ocelot.c

> +++ b/net/dsa/tag_ocelot.c

> @@ -160,11 +160,14 @@ static struct sk_buff *ocelot_xmit(struct sk_buff *skb,

>  	packing(injection, &qos_class, 19,  17, OCELOT_TAG_LEN, PACK, 0);

>  

>  	if (ocelot->ptp && (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP)) {

> +		struct sk_buff *clone = DSA_SKB_CB(skb)->clone;

> +

>  		rew_op = ocelot_port->ptp_cmd;

> -		if (ocelot_port->ptp_cmd == IFH_REW_OP_TWO_STEP_PTP) {

> -			rew_op |= (ocelot_port->ts_id  % 4) << 3;

> -			ocelot_port->ts_id++;

> -		}

> +		/* Retrieve timestamp ID populated inside skb->cb[0] of the

> +		 * clone by ocelot_port_add_txtstamp_skb

> +		 */

> +		if (ocelot_port->ptp_cmd == IFH_REW_OP_TWO_STEP_PTP)

> +			rew_op |= clone->cb[0] << 3;

>  

>  		packing(injection, &rew_op, 125, 117, OCELOT_TAG_LEN, PACK, 0);

>  	}

> -- 

> 2.25.1

> 


-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
diff mbox series

Patch

diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
index 5abb7d2b0a9e..83eb7c325061 100644
--- a/drivers/net/ethernet/mscc/ocelot.c
+++ b/drivers/net/ethernet/mscc/ocelot.c
@@ -421,10 +421,15 @@  int ocelot_port_add_txtstamp_skb(struct ocelot_port *ocelot_port,
 
 	if (ocelot->ptp && shinfo->tx_flags & SKBTX_HW_TSTAMP &&
 	    ocelot_port->ptp_cmd == IFH_REW_OP_TWO_STEP_PTP) {
+		spin_lock(&ocelot_port->ts_id_lock);
+
 		shinfo->tx_flags |= SKBTX_IN_PROGRESS;
 		/* Store timestamp ID in cb[0] of sk_buff */
-		skb->cb[0] = ocelot_port->ts_id % 4;
+		skb->cb[0] = ocelot_port->ts_id;
+		ocelot_port->ts_id = (ocelot_port->ts_id + 1) % 4;
 		skb_queue_tail(&ocelot_port->tx_skbs, skb);
+
+		spin_unlock(&ocelot_port->ts_id_lock);
 		return 0;
 	}
 	return -ENODATA;
@@ -1300,6 +1305,7 @@  void ocelot_init_port(struct ocelot *ocelot, int port)
 	struct ocelot_port *ocelot_port = ocelot->ports[port];
 
 	skb_queue_head_init(&ocelot_port->tx_skbs);
+	spin_lock_init(&ocelot_port->ts_id_lock);
 
 	/* Basic L2 initialization */
 
diff --git a/drivers/net/ethernet/mscc/ocelot_net.c b/drivers/net/ethernet/mscc/ocelot_net.c
index cacabc23215a..8490e42e9e2d 100644
--- a/drivers/net/ethernet/mscc/ocelot_net.c
+++ b/drivers/net/ethernet/mscc/ocelot_net.c
@@ -349,10 +349,8 @@  static int ocelot_port_xmit(struct sk_buff *skb, struct net_device *dev)
 
 	if (ocelot->ptp && shinfo->tx_flags & SKBTX_HW_TSTAMP) {
 		info.rew_op = ocelot_port->ptp_cmd;
-		if (ocelot_port->ptp_cmd == IFH_REW_OP_TWO_STEP_PTP) {
-			info.rew_op |= (ocelot_port->ts_id  % 4) << 3;
-			ocelot_port->ts_id++;
-		}
+		if (ocelot_port->ptp_cmd == IFH_REW_OP_TWO_STEP_PTP)
+			info.rew_op |= skb->cb[0] << 3;
 	}
 
 	ocelot_gen_ifh(ifh, &info);
diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h
index da369b12005f..4521dd602ddc 100644
--- a/include/soc/mscc/ocelot.h
+++ b/include/soc/mscc/ocelot.h
@@ -566,6 +566,7 @@  struct ocelot_port {
 	u8				ptp_cmd;
 	struct sk_buff_head		tx_skbs;
 	u8				ts_id;
+	spinlock_t			ts_id_lock;
 
 	phy_interface_t			phy_mode;
 
diff --git a/net/dsa/tag_ocelot.c b/net/dsa/tag_ocelot.c
index 42f327c06dca..b4fc05cafaa6 100644
--- a/net/dsa/tag_ocelot.c
+++ b/net/dsa/tag_ocelot.c
@@ -160,11 +160,14 @@  static struct sk_buff *ocelot_xmit(struct sk_buff *skb,
 	packing(injection, &qos_class, 19,  17, OCELOT_TAG_LEN, PACK, 0);
 
 	if (ocelot->ptp && (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP)) {
+		struct sk_buff *clone = DSA_SKB_CB(skb)->clone;
+
 		rew_op = ocelot_port->ptp_cmd;
-		if (ocelot_port->ptp_cmd == IFH_REW_OP_TWO_STEP_PTP) {
-			rew_op |= (ocelot_port->ts_id  % 4) << 3;
-			ocelot_port->ts_id++;
-		}
+		/* Retrieve timestamp ID populated inside skb->cb[0] of the
+		 * clone by ocelot_port_add_txtstamp_skb
+		 */
+		if (ocelot_port->ptp_cmd == IFH_REW_OP_TWO_STEP_PTP)
+			rew_op |= clone->cb[0] << 3;
 
 		packing(injection, &rew_op, 125, 117, OCELOT_TAG_LEN, PACK, 0);
 	}