2021-06-07 03:17:39

by Steven Ting

[permalink] [raw]
Subject: Is the extra_tx_headroom guarenteed ?

Johannes and all mac80211 gurus,

We encountered a problem that we use the extra_tx_headroom to reserve the headroom
which we put the txdesc in.

Current workaround is that we check our needed headroom size by skb_headroom()
in the driver layer.

Is extra_tx_headroom in struct ieee80211_hw always guaranteed?
The header file describes:
* @extra_tx_headroom: headroom to reserve in each transmit skb
* for use by the driver (e.g. for transmit headers.)

But when the skb goes through the ieee80211_amsdu_realloc_pad(), it does not
take care of the extra_tx_headroom, i.e. the original reserved headroom might be
eaten.

Does the ieee80211_amsdu_realloc_pad() lacks some check for extra_tx_headroom
or the extra_tx_headroom in mac80211 is not guaranteed?

Furthermore, for the packet that would not be aggregate in A-MSDU and ndev->needed_headroom
is not guaranteed, in this case whether mac80211 layer still guarantee the extra_tx_headroom ?

Or mac80211 only guarantees the headroom of the skb which is built by itself ?

Steven


2021-06-10 20:16:26

by Johannes Berg

[permalink] [raw]
Subject: Re: Is the extra_tx_headroom guarenteed ?

Hi,

> We encountered a problem that we use the extra_tx_headroom to reserve the headroom
> which we put the txdesc in.
>
> Current workaround is that we check our needed headroom size by skb_headroom()
> in the driver layer.
>
> Is extra_tx_headroom in struct ieee80211_hw always guaranteed?

It _should_ be, IMHO. Having the check in all the drivers would be
pointless.

> The header file describes:
> * @extra_tx_headroom: headroom to reserve in each transmit skb
> * for use by the driver (e.g. for transmit headers.)
>
> But when the skb goes through the ieee80211_amsdu_realloc_pad(), it does not
> take care of the extra_tx_headroom, i.e. the original reserved headroom might be
> eaten.

OK, so I guess that's a bug there.

> Does the ieee80211_amsdu_realloc_pad() lacks some check for extra_tx_headroom
> or the extra_tx_headroom in mac80211 is not guaranteed?

I would say it lacks the checks - want to send a patch?

> Furthermore, for the packet that would not be aggregate in A-MSDU and ndev->needed_headroom
> is not guaranteed, in this case whether mac80211 layer still guarantee the extra_tx_headroom ?

Yes, this case should be handled.

johannes

2021-08-16 08:57:49

by Ping-Ke Shih

[permalink] [raw]
Subject: RE: Is the extra_tx_headroom guarenteed ?


> -----Original Message-----
> From: Johannes Berg [mailto:[email protected]]
> Sent: Friday, June 11, 2021 4:14 AM
> To: Steven Ting
> Cc: [email protected]
> Subject: Re: Is the extra_tx_headroom guarenteed ?
>
> Hi,
>
> > We encountered a problem that we use the extra_tx_headroom to reserve the headroom
> > which we put the txdesc in.
> >
> > Current workaround is that we check our needed headroom size by skb_headroom()
> > in the driver layer.
> >
> > Is extra_tx_headroom in struct ieee80211_hw always guaranteed?
>
> It _should_ be, IMHO. Having the check in all the drivers would be
> pointless.
>
> > The header file describes:
> > * @extra_tx_headroom: headroom to reserve in each transmit skb
> > * for use by the driver (e.g. for transmit headers.)
> >
> > But when the skb goes through the ieee80211_amsdu_realloc_pad(), it does not
> > take care of the extra_tx_headroom, i.e. the original reserved headroom might be
> > eaten.
>
> OK, so I guess that's a bug there.
>
> > Does the ieee80211_amsdu_realloc_pad() lacks some check for extra_tx_headroom
> > or the extra_tx_headroom in mac80211 is not guaranteed?
>
> I would say it lacks the checks - want to send a patch?
>
> > Furthermore, for the packet that would not be aggregate in A-MSDU and ndev->needed_headroom
> > is not guaranteed, in this case whether mac80211 layer still guarantee the extra_tx_headroom ?
>
> Yes, this case should be handled.
>

We prepare a patchset [1] to fix the bug mentioned in this mail.

[1] https://lore.kernel.org/linux-wireless/[email protected]/T/#t

--
Ping-Ke