* [PATCH] net: mscc: ocelot: fix potential issues accessing skbs list
@ 2019-11-26 10:44 Yangbo Lu
2019-11-26 12:31 ` Vladimir Oltean
2019-11-26 22:01 ` David Miller
0 siblings, 2 replies; 4+ messages in thread
From: Yangbo Lu @ 2019-11-26 10:44 UTC (permalink / raw)
To: netdev, Alexandre Belloni, Microchip Linux Driver Support,
David S . Miller
Cc: Yangbo Lu
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(+)
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;
};
--
2.7.4
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] net: mscc: ocelot: fix potential issues accessing skbs list
2019-11-26 10:44 [PATCH] net: mscc: ocelot: fix potential issues accessing skbs list Yangbo Lu
@ 2019-11-26 12:31 ` Vladimir Oltean
2019-11-27 8:08 ` Y.b. Lu
2019-11-26 22:01 ` David Miller
1 sibling, 1 reply; 4+ messages in thread
From: Vladimir Oltean @ 2019-11-26 12:31 UTC (permalink / raw)
To: Yangbo Lu
Cc: netdev, Alexandre Belloni, Microchip Linux Driver Support,
David S . Miller
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] net: mscc: ocelot: fix potential issues accessing skbs list
2019-11-26 10:44 [PATCH] net: mscc: ocelot: fix potential issues accessing skbs list Yangbo Lu
2019-11-26 12:31 ` Vladimir Oltean
@ 2019-11-26 22:01 ` David Miller
1 sibling, 0 replies; 4+ messages in thread
From: David Miller @ 2019-11-26 22:01 UTC (permalink / raw)
To: yangbo.lu; +Cc: netdev, alexandre.belloni, UNGLinuxDriver
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.
^ permalink raw reply [flat|nested] 4+ messages in thread
* RE: [PATCH] net: mscc: ocelot: fix potential issues accessing skbs list
2019-11-26 12:31 ` Vladimir Oltean
@ 2019-11-27 8:08 ` Y.b. Lu
0 siblings, 0 replies; 4+ messages in thread
From: Y.b. Lu @ 2019-11-27 8:08 UTC (permalink / raw)
To: Vladimir Oltean, David S . Miller
Cc: netdev, Alexandre Belloni, Microchip Linux Driver Support
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-11-27 8:08 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-11-26 10:44 [PATCH] net: mscc: ocelot: fix potential issues accessing skbs list Yangbo Lu
2019-11-26 12:31 ` Vladimir Oltean
2019-11-27 8:08 ` Y.b. Lu
2019-11-26 22:01 ` David Miller
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).