netdev - Re: [PATCH wpan-next 08/11] net: mac802154: Add a warning in the hot path

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 28 Apr 2022 09:58:48 +0200
From:   Miquel Raynal <miquel.raynal@...tlin.com>
To:     Alexander Aring <alex.aring@...il.com>
Cc:     Stefan Schmidt <stefan@...enfreihafen.org>,
        linux-wpan - ML <linux-wpan@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        "open list:NETWORKING [GENERAL]" <netdev@...r.kernel.org>,
        David Girault <david.girault@...vo.com>,
        Romuald Despres <romuald.despres@...vo.com>,
        Frederic Blain <frederic.blain@...vo.com>,
        Nicolas Schodet <nico@...fr.eu.org>,
        Thomas Petazzoni <thomas.petazzoni@...tlin.com>
Subject: Re: [PATCH wpan-next 08/11] net: mac802154: Add a warning in the
 hot path

Hi Alexander,

alex.aring@...il.com wrote on Wed, 27 Apr 2022 14:01:25 -0400:

> Hi,
> 
> On Wed, Apr 27, 2022 at 12:47 PM Miquel Raynal
> <miquel.raynal@...tlin.com> wrote:
> >
> > We should never start a transmission after the queue has been stopped.
> >
> > But because it might work we don't kill the function here but rather
> > warn loudly the user that something is wrong.
> >
> > Signed-off-by: Miquel Raynal <miquel.raynal@...tlin.com>
> > ---

[...]

> > diff --git a/net/mac802154/tx.c b/net/mac802154/tx.c
> > index a8a83f0167bf..021dddfea542 100644
> > --- a/net/mac802154/tx.c
> > +++ b/net/mac802154/tx.c
> > @@ -124,6 +124,8 @@ bool ieee802154_queue_is_held(struct ieee802154_local *local)
> >  static netdev_tx_t
> >  ieee802154_hot_tx(struct ieee802154_local *local, struct sk_buff *skb)
> >  {
> > +       WARN_ON_ONCE(ieee802154_queue_is_stopped(local));
> > +
> >         return ieee802154_tx(local, skb);
> >  }
> >
> > diff --git a/net/mac802154/util.c b/net/mac802154/util.c
> > index 847e0864b575..cfd17a7db532 100644
> > --- a/net/mac802154/util.c
> > +++ b/net/mac802154/util.c
> > @@ -44,6 +44,24 @@ void ieee802154_stop_queue(struct ieee802154_local *local)
> >         rcu_read_unlock();
> >  }
> >
> > +bool ieee802154_queue_is_stopped(struct ieee802154_local *local)
> > +{
> > +       struct ieee802154_sub_if_data *sdata;
> > +       bool stopped = true;
> > +
> > +       rcu_read_lock();
> > +       list_for_each_entry_rcu(sdata, &local->interfaces, list) {
> > +               if (!sdata->dev)
> > +                       continue;
> > +
> > +               if (!netif_queue_stopped(sdata->dev))
> > +                       stopped = false;
> > +       }
> > +       rcu_read_unlock();
> > +
> > +       return stopped;
> > +}  
> 
> sorry this makes no sense, you using net core functionality to check
> if a queue is stopped in a net core netif callback. Whereas the sense
> here for checking if the queue is really stopped is when 802.15.4
> thinks the queue is stopped vs net core netif callback running. It
> means for MLME-ops there are points we want to make sure that net core
> is not handling any xmit and we should check this point and not
> introducing net core functionality checks.

I think I've mixed two things, your remark makes complete sense. I
should instead here just check a 802.15.4 internal variable.

> btw: if it's hit your if branch the first time you can break?

Yes, we could definitely improve a bit the logic to break earlier, but
in the end these checks won't remain I believe.

> I am not done with the review, this is just what I see now and we can
> discuss that. Please be patient.

Sure, thanks for the quick feedback anyway!

hanks,
Miquèl