Is the extra_tx_headroom guarenteed ?

Is the extra_tx_headroom guarenteed ?

[Steven Ting]

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

Re: Is the extra_tx_headroom guarenteed ?

[Johannes Berg]

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

RE: Is the extra_tx_headroom guarenteed ?

[Pkshih]

> -----Original Message-----
> From: Johannes Berg [mailto:johannes@sipsolutions.net]
> Sent: Friday, June 11, 2021 4:14 AM
> To: Steven Ting
> Cc: linux-wireless@vger.kernel.org
> 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/20210816085128.10931-1-pkshih@realtek.com/T/#t

--
Ping-Ke