| 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 |
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
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 --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); }