net: mscc: ocelot: fix potential issues accessing skbs list
diff mbox series

Message ID 20191126104403.46717-1-yangbo.lu@nxp.com
State Changes Requested
Delegated to: David Miller
Headers show
Series
  • net: mscc: ocelot: fix potential issues accessing skbs list
Related show

Commit Message

Y.b. Lu Nov. 26, 2019, 10:44 a.m. UTC
Fix two prtential issues accessing skbs list.
- Protect skbs list in case of competing for accessing.
- Break the matching loop when find the matching skb to
  avoid consuming more skbs incorrectly. The ID is only
  from 0 to 3, but the FIFO supports 128 timestamps at most.

Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
---
 drivers/net/ethernet/mscc/ocelot.c | 11 +++++++++++
 include/soc/mscc/ocelot.h          |  2 ++
 2 files changed, 13 insertions(+)

Comments

Vladimir Oltean Nov. 26, 2019, 12:31 p.m. UTC | #1
Hi Yangbo,

On Tue, 26 Nov 2019 at 14:22, Yangbo Lu <yangbo.lu@nxp.com> wrote:
>
> Fix two prtential issues accessing skbs list.
> - Protect skbs list in case of competing for accessing.
> - Break the matching loop when find the matching skb to
>   avoid consuming more skbs incorrectly. The ID is only
>   from 0 to 3, but the FIFO supports 128 timestamps at most.
>
> Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
> ---

Don't you want to convert struct ocelot_skb to a proper struct
sk_buff_head skb_queue, which has its own lock when calling
skb_dequeue? The timestamp ID can be held in skb->cb.

>  drivers/net/ethernet/mscc/ocelot.c | 11 +++++++++++
>  include/soc/mscc/ocelot.h          |  2 ++
>  2 files changed, 13 insertions(+)
>
> diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
> index 0e96ffa..5d842d8 100644
> --- a/drivers/net/ethernet/mscc/ocelot.c
> +++ b/drivers/net/ethernet/mscc/ocelot.c
> @@ -580,6 +580,7 @@ int ocelot_port_add_txtstamp_skb(struct ocelot_port *ocelot_port,
>  {
>         struct skb_shared_info *shinfo = skb_shinfo(skb);
>         struct ocelot *ocelot = ocelot_port->ocelot;
> +       unsigned long flags;
>
>         if (ocelot->ptp && shinfo->tx_flags & SKBTX_HW_TSTAMP &&
>             ocelot_port->ptp_cmd == IFH_REW_OP_TWO_STEP_PTP) {
> @@ -594,7 +595,9 @@ int ocelot_port_add_txtstamp_skb(struct ocelot_port *ocelot_port,
>                 oskb->skb = skb;
>                 oskb->id = ocelot_port->ts_id % 4;
>
> +               spin_lock_irqsave(&ocelot_port->skbs_lock, flags);
>                 list_add_tail(&oskb->head, &ocelot_port->skbs);
> +               spin_unlock_irqrestore(&ocelot_port->skbs_lock, flags);
>                 return 0;
>         }
>         return -ENODATA;
> @@ -702,6 +705,7 @@ static void ocelot_get_hwtimestamp(struct ocelot *ocelot,
>  void ocelot_get_txtstamp(struct ocelot *ocelot)
>  {
>         int budget = OCELOT_PTP_QUEUE_SZ;
> +       unsigned long flags;
>
>         while (budget--) {
>                 struct skb_shared_hwtstamps shhwtstamps;
> @@ -727,6 +731,7 @@ void ocelot_get_txtstamp(struct ocelot *ocelot)
>                 /* Retrieve its associated skb */
>                 port = ocelot->ports[txport];
>
> +               spin_lock_irqsave(&port->skbs_lock, flags);
>                 list_for_each_safe(pos, tmp, &port->skbs) {
>                         entry = list_entry(pos, struct ocelot_skb, head);
>                         if (entry->id != id)
> @@ -736,7 +741,9 @@ void ocelot_get_txtstamp(struct ocelot *ocelot)
>
>                         list_del(pos);
>                         kfree(entry);
> +                       break;

I think this should be a separate patch, since it is unrelated to locking.

>                 }
> +               spin_unlock_irqrestore(&port->skbs_lock, flags);
>
>                 /* Next ts */
>                 ocelot_write(ocelot, SYS_PTP_NXT_PTP_NXT, SYS_PTP_NXT);
> @@ -2205,6 +2212,7 @@ void ocelot_init_port(struct ocelot *ocelot, int port)
>  {
>         struct ocelot_port *ocelot_port = ocelot->ports[port];
>
> +       spin_lock_init(&ocelot_port->skbs_lock);
>         INIT_LIST_HEAD(&ocelot_port->skbs);
>
>         /* Basic L2 initialization */
> @@ -2493,6 +2501,7 @@ void ocelot_deinit(struct ocelot *ocelot)
>         struct list_head *pos, *tmp;
>         struct ocelot_port *port;
>         struct ocelot_skb *entry;
> +       unsigned long flags;
>         int i;
>
>         cancel_delayed_work(&ocelot->stats_work);
> @@ -2503,6 +2512,7 @@ void ocelot_deinit(struct ocelot *ocelot)
>         for (i = 0; i < ocelot->num_phys_ports; i++) {
>                 port = ocelot->ports[i];
>
> +               spin_lock_irqsave(&port->skbs_lock, flags);
>                 list_for_each_safe(pos, tmp, &port->skbs) {
>                         entry = list_entry(pos, struct ocelot_skb, head);
>
> @@ -2510,6 +2520,7 @@ void ocelot_deinit(struct ocelot *ocelot)
>                         dev_kfree_skb_any(entry->skb);
>                         kfree(entry);
>                 }
> +               spin_unlock_irqrestore(&port->skbs_lock, flags);
>         }
>  }
>  EXPORT_SYMBOL(ocelot_deinit);
> diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h
> index e1108a5..7973458 100644
> --- a/include/soc/mscc/ocelot.h
> +++ b/include/soc/mscc/ocelot.h
> @@ -426,6 +426,8 @@ struct ocelot_port {
>
>         u8                              ptp_cmd;
>         struct list_head                skbs;
> +       /* Protects the skbs list */
> +       spinlock_t                      skbs_lock;
>         u8                              ts_id;
>  };
>
> --
> 2.7.4
>

Regards,
-Vladimir
David Miller Nov. 26, 2019, 10:01 p.m. UTC | #2
From: Yangbo Lu <yangbo.lu@nxp.com>
Date: Tue, 26 Nov 2019 18:44:03 +0800

> Fix two prtential issues accessing skbs list.
> - Protect skbs list in case of competing for accessing.
> - Break the matching loop when find the matching skb to
>   avoid consuming more skbs incorrectly. The ID is only
>   from 0 to 3, but the FIFO supports 128 timestamps at most.
> 
> Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>

Seriously, use skb_queue_head and associated helpers, we have
all of the infrastructure and arrangements for this.
Y.b. Lu Nov. 27, 2019, 8:08 a.m. UTC | #3
Thanks Vladimir and David.
I hadn't known such skb queue before. Sent out v2.

Best regards,
Yangbo Lu

> -----Original Message-----
> From: Vladimir Oltean <olteanv@gmail.com>
> Sent: Tuesday, November 26, 2019 8:32 PM
> To: Y.b. Lu <yangbo.lu@nxp.com>
> Cc: netdev <netdev@vger.kernel.org>; Alexandre Belloni
> <alexandre.belloni@bootlin.com>; Microchip Linux Driver Support
> <UNGLinuxDriver@microchip.com>; David S . Miller <davem@davemloft.net>
> Subject: Re: [PATCH] net: mscc: ocelot: fix potential issues accessing skbs list
> 
> Hi Yangbo,
> 
> On Tue, 26 Nov 2019 at 14:22, Yangbo Lu <yangbo.lu@nxp.com> wrote:
> >
> > Fix two prtential issues accessing skbs list.
> > - Protect skbs list in case of competing for accessing.
> > - Break the matching loop when find the matching skb to
> >   avoid consuming more skbs incorrectly. The ID is only
> >   from 0 to 3, but the FIFO supports 128 timestamps at most.
> >
> > Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
> > ---
> 
> Don't you want to convert struct ocelot_skb to a proper struct sk_buff_head
> skb_queue, which has its own lock when calling skb_dequeue? The timestamp
> ID can be held in skb->cb.
> 
> >  drivers/net/ethernet/mscc/ocelot.c | 11 +++++++++++
> >  include/soc/mscc/ocelot.h          |  2 ++
> >  2 files changed, 13 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/mscc/ocelot.c
> > b/drivers/net/ethernet/mscc/ocelot.c
> > index 0e96ffa..5d842d8 100644
> > --- a/drivers/net/ethernet/mscc/ocelot.c
> > +++ b/drivers/net/ethernet/mscc/ocelot.c
> > @@ -580,6 +580,7 @@ int ocelot_port_add_txtstamp_skb(struct
> > ocelot_port *ocelot_port,  {
> >         struct skb_shared_info *shinfo = skb_shinfo(skb);
> >         struct ocelot *ocelot = ocelot_port->ocelot;
> > +       unsigned long flags;
> >
> >         if (ocelot->ptp && shinfo->tx_flags & SKBTX_HW_TSTAMP &&
> >             ocelot_port->ptp_cmd == IFH_REW_OP_TWO_STEP_PTP)
> { @@
> > -594,7 +595,9 @@ int ocelot_port_add_txtstamp_skb(struct ocelot_port
> *ocelot_port,
> >                 oskb->skb = skb;
> >                 oskb->id = ocelot_port->ts_id % 4;
> >
> > +               spin_lock_irqsave(&ocelot_port->skbs_lock, flags);
> >                 list_add_tail(&oskb->head, &ocelot_port->skbs);
> > +               spin_unlock_irqrestore(&ocelot_port->skbs_lock,
> > + flags);
> >                 return 0;
> >         }
> >         return -ENODATA;
> > @@ -702,6 +705,7 @@ static void ocelot_get_hwtimestamp(struct ocelot
> > *ocelot,  void ocelot_get_txtstamp(struct ocelot *ocelot)  {
> >         int budget = OCELOT_PTP_QUEUE_SZ;
> > +       unsigned long flags;
> >
> >         while (budget--) {
> >                 struct skb_shared_hwtstamps shhwtstamps; @@ -727,6
> > +731,7 @@ void ocelot_get_txtstamp(struct ocelot *ocelot)
> >                 /* Retrieve its associated skb */
> >                 port = ocelot->ports[txport];
> >
> > +               spin_lock_irqsave(&port->skbs_lock, flags);
> >                 list_for_each_safe(pos, tmp, &port->skbs) {
> >                         entry = list_entry(pos, struct ocelot_skb, head);
> >                         if (entry->id != id) @@ -736,7 +741,9 @@ void
> > ocelot_get_txtstamp(struct ocelot *ocelot)
> >
> >                         list_del(pos);
> >                         kfree(entry);
> > +                       break;
> 
> I think this should be a separate patch, since it is unrelated to locking.
> 
> >                 }
> > +               spin_unlock_irqrestore(&port->skbs_lock, flags);
> >
> >                 /* Next ts */
> >                 ocelot_write(ocelot, SYS_PTP_NXT_PTP_NXT,
> > SYS_PTP_NXT); @@ -2205,6 +2212,7 @@ void ocelot_init_port(struct
> > ocelot *ocelot, int port)  {
> >         struct ocelot_port *ocelot_port = ocelot->ports[port];
> >
> > +       spin_lock_init(&ocelot_port->skbs_lock);
> >         INIT_LIST_HEAD(&ocelot_port->skbs);
> >
> >         /* Basic L2 initialization */
> > @@ -2493,6 +2501,7 @@ void ocelot_deinit(struct ocelot *ocelot)
> >         struct list_head *pos, *tmp;
> >         struct ocelot_port *port;
> >         struct ocelot_skb *entry;
> > +       unsigned long flags;
> >         int i;
> >
> >         cancel_delayed_work(&ocelot->stats_work);
> > @@ -2503,6 +2512,7 @@ void ocelot_deinit(struct ocelot *ocelot)
> >         for (i = 0; i < ocelot->num_phys_ports; i++) {
> >                 port = ocelot->ports[i];
> >
> > +               spin_lock_irqsave(&port->skbs_lock, flags);
> >                 list_for_each_safe(pos, tmp, &port->skbs) {
> >                         entry = list_entry(pos, struct ocelot_skb,
> > head);
> >
> > @@ -2510,6 +2520,7 @@ void ocelot_deinit(struct ocelot *ocelot)
> >                         dev_kfree_skb_any(entry->skb);
> >                         kfree(entry);
> >                 }
> > +               spin_unlock_irqrestore(&port->skbs_lock, flags);
> >         }
> >  }
> >  EXPORT_SYMBOL(ocelot_deinit);
> > diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h
> > index e1108a5..7973458 100644
> > --- a/include/soc/mscc/ocelot.h
> > +++ b/include/soc/mscc/ocelot.h
> > @@ -426,6 +426,8 @@ struct ocelot_port {
> >
> >         u8                              ptp_cmd;
> >         struct list_head                skbs;
> > +       /* Protects the skbs list */
> > +       spinlock_t                      skbs_lock;
> >         u8                              ts_id;
> >  };
> >
> > --
> > 2.7.4
> >
> 
> Regards,
> -Vladimir

Patch
diff mbox series

diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
index 0e96ffa..5d842d8 100644
--- a/drivers/net/ethernet/mscc/ocelot.c
+++ b/drivers/net/ethernet/mscc/ocelot.c
@@ -580,6 +580,7 @@  int ocelot_port_add_txtstamp_skb(struct ocelot_port *ocelot_port,
 {
 	struct skb_shared_info *shinfo = skb_shinfo(skb);
 	struct ocelot *ocelot = ocelot_port->ocelot;
+	unsigned long flags;
 
 	if (ocelot->ptp && shinfo->tx_flags & SKBTX_HW_TSTAMP &&
 	    ocelot_port->ptp_cmd == IFH_REW_OP_TWO_STEP_PTP) {
@@ -594,7 +595,9 @@  int ocelot_port_add_txtstamp_skb(struct ocelot_port *ocelot_port,
 		oskb->skb = skb;
 		oskb->id = ocelot_port->ts_id % 4;
 
+		spin_lock_irqsave(&ocelot_port->skbs_lock, flags);
 		list_add_tail(&oskb->head, &ocelot_port->skbs);
+		spin_unlock_irqrestore(&ocelot_port->skbs_lock, flags);
 		return 0;
 	}
 	return -ENODATA;
@@ -702,6 +705,7 @@  static void ocelot_get_hwtimestamp(struct ocelot *ocelot,
 void ocelot_get_txtstamp(struct ocelot *ocelot)
 {
 	int budget = OCELOT_PTP_QUEUE_SZ;
+	unsigned long flags;
 
 	while (budget--) {
 		struct skb_shared_hwtstamps shhwtstamps;
@@ -727,6 +731,7 @@  void ocelot_get_txtstamp(struct ocelot *ocelot)
 		/* Retrieve its associated skb */
 		port = ocelot->ports[txport];
 
+		spin_lock_irqsave(&port->skbs_lock, flags);
 		list_for_each_safe(pos, tmp, &port->skbs) {
 			entry = list_entry(pos, struct ocelot_skb, head);
 			if (entry->id != id)
@@ -736,7 +741,9 @@  void ocelot_get_txtstamp(struct ocelot *ocelot)
 
 			list_del(pos);
 			kfree(entry);
+			break;
 		}
+		spin_unlock_irqrestore(&port->skbs_lock, flags);
 
 		/* Next ts */
 		ocelot_write(ocelot, SYS_PTP_NXT_PTP_NXT, SYS_PTP_NXT);
@@ -2205,6 +2212,7 @@  void ocelot_init_port(struct ocelot *ocelot, int port)
 {
 	struct ocelot_port *ocelot_port = ocelot->ports[port];
 
+	spin_lock_init(&ocelot_port->skbs_lock);
 	INIT_LIST_HEAD(&ocelot_port->skbs);
 
 	/* Basic L2 initialization */
@@ -2493,6 +2501,7 @@  void ocelot_deinit(struct ocelot *ocelot)
 	struct list_head *pos, *tmp;
 	struct ocelot_port *port;
 	struct ocelot_skb *entry;
+	unsigned long flags;
 	int i;
 
 	cancel_delayed_work(&ocelot->stats_work);
@@ -2503,6 +2512,7 @@  void ocelot_deinit(struct ocelot *ocelot)
 	for (i = 0; i < ocelot->num_phys_ports; i++) {
 		port = ocelot->ports[i];
 
+		spin_lock_irqsave(&port->skbs_lock, flags);
 		list_for_each_safe(pos, tmp, &port->skbs) {
 			entry = list_entry(pos, struct ocelot_skb, head);
 
@@ -2510,6 +2520,7 @@  void ocelot_deinit(struct ocelot *ocelot)
 			dev_kfree_skb_any(entry->skb);
 			kfree(entry);
 		}
+		spin_unlock_irqrestore(&port->skbs_lock, flags);
 	}
 }
 EXPORT_SYMBOL(ocelot_deinit);
diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h
index e1108a5..7973458 100644
--- a/include/soc/mscc/ocelot.h
+++ b/include/soc/mscc/ocelot.h
@@ -426,6 +426,8 @@  struct ocelot_port {
 
 	u8				ptp_cmd;
 	struct list_head		skbs;
+	/* Protects the skbs list */
+	spinlock_t			skbs_lock;
 	u8				ts_id;
 };