netdev - Re: [PATCH wpan-next v2 0/2] ieee802154: Beaconing support

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]

Message-ID: <CAK-6q+hAgyx3YML7Lw=MAkUX4i8PVqxSKiVzeAM-wGJOdL9aXA@mail.gmail.com>
Date:   Wed, 1 Feb 2023 12:15:42 -0500
From:   Alexander Aring <aahringo@...hat.com>
To:     Miquel Raynal <miquel.raynal@...tlin.com>
Cc:     Alexander Aring <alex.aring@...il.com>,
        Stefan Schmidt <stefan@...enfreihafen.org>,
        linux-wpan@...r.kernel.org,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Eric Dumazet <edumazet@...gle.com>, 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>,
        Guilhem Imberton <guilhem.imberton@...vo.com>,
        Thomas Petazzoni <thomas.petazzoni@...tlin.com>
Subject: Re: [PATCH wpan-next v2 0/2] ieee802154: Beaconing support

Hi,

On Tue, Jan 31, 2023 at 6:25 AM Miquel Raynal <miquel.raynal@...tlin.com> wrote:
>
> Hi Alexander,
>
> > > > > > Changes in v2:
> > > > > > * Clearly state in the commit log llsec is not supported yet.
> > > > > > * Do not use mlme transmission helpers because we don't really need to
> > > > > >   stop the queue when sending a beacon, as we don't expect any feedback
> > > > > >   from the PHY nor from the peers. However, we don't want to go through
> > > > > >   the whole net stack either, so we bypass it calling the subif helper
> > > > > >   directly.
> > > > > >
> > > >
> > > > moment, we use the mlme helpers to stop tx
> > >
> > > No, we no longer use the mlme helpers to stop tx when sending beacons
> > > (but true MLME transmissions, we ack handling and return codes will be
> > > used for other purposes).
> > >
> >
> > then we run into an issue overwriting the framebuffer while the normal
> > transmit path is active?
>
> Crap, yes you're right. That's not gonna work.
>
> The net core acquires HARD_TX_LOCK() to avoid these issues and we are
> no bypassing the net core without taking care of the proper frame
> transmissions either (which would have worked with mlme_tx_one()). So I
> guess there are two options:
>
> * Either we deal with the extra penalty of stopping the queue and
>   waiting for the beacon to be transmitted with an mlme_tx_one() call,
>   as proposed initially.
>
> * Or we hardcode our own "net" transmit helper, something like:
>
> mac802154_fast_mlme_tx() {
>         struct net_device *dev = skb->dev;
>         struct netdev_queue *txq;
>
>         txq = netdev_core_pick_tx(dev, skb, NULL);
>         cpu = smp_processor_id();
>         HARD_TX_LOCK(dev, txq, cpu);
>         if (!netif_xmit_frozen_or_drv_stopped(txq))
>                 netdev_start_xmit(skb, dev, txq, 0);
>         HARD_TX_UNLOCK(dev, txq);
> }
>
> Note1: this is very close to generic_xdp_tx() which tries to achieve the
> same goal: sending packets, bypassing qdisc et al. I don't know whether
> it makes sense to define it under mac802154/tx.c or core/dev.c and give
> it another name, like generic_tx() or whatever would be more
> appropriate. Or even adapting generic_xdp_tx() to make it look more
> generic and use that function instead (without the xdp struct pointer).
>

The problem here is that the transmit handling is completely
asynchronous. Calling netdev_start_xmit() is not "transmit and wait
until transmit is done", it is "start transmit here is the buffer" an
interrupt is coming up to report transmit is done. Until the time the
interrupt isn't arrived the framebuffer on the device is in use, we
don't know when the transceiver is done reading it. Only after tx done
isr. The time until the isr isn't arrived is for us a -EBUSY case due
hardware resource limitation. Currently we do that with stop/wake
queue to avoid calling of xmit_do() to not run into such -EBUSY
cases...

There might be clever things to do here to avoid this issue... I am
not sure how XDP does that.

> Note2: I am wondering if it makes sense to disable bh here as well?

May HARD_TX_LOCK() already do that? If they use spin_lock_bh() they
disable local softirqs until the lock isn't held anymore.

>
> Once we settle, I send a patch.
>

Not sure how to preceded here, but do see the problem? Or maybe I
overlooked something here...

- Alex