| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <VI1PR0401MB2237E5D4BA75F11206F0770DF8440@VI1PR0401MB2237.eurprd04.prod.outlook.com>
Date: Wed, 27 Nov 2019 08:08:40 +0000
From: "Y.b. Lu" <yangbo.lu@....com>
To: Vladimir Oltean <olteanv@...il.com>,
"David S . Miller" <davem@...emloft.net>
CC: netdev <netdev@...r.kernel.org>,
Alexandre Belloni <alexandre.belloni@...tlin.com>,
Microchip Linux Driver Support <UNGLinuxDriver@...rochip.com>
Subject: RE: [PATCH] net: mscc: ocelot: fix potential issues accessing skbs
list
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@...il.com>
> Sent: Tuesday, November 26, 2019 8:32 PM
> To: Y.b. Lu <yangbo.lu@....com>
> Cc: netdev <netdev@...r.kernel.org>; Alexandre Belloni
> <alexandre.belloni@...tlin.com>; Microchip Linux Driver Support
> <UNGLinuxDriver@...rochip.com>; David S . Miller <davem@...emloft.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@....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@....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
Powered by blists - more mailing lists