+ johannes Erik Stromdahl <erik.stromdahl@xxxxxxxxx> writes: > On 4/12/19 3:17 PM, Kalle Valo wrote: >> Erik Stromdahl <erik.stromdahl@xxxxxxxxx> writes: >> >>> This patch fixes a bug with padding of the skb data buffer. >>> Since skb_trim can only be used to reduce the skb len, it is useless when >>> we pad (increase the length of) the skb. Instead we must set skb->len >>> directly. >>> >>> Signed-off-by: Erik Stromdahl <erik.stromdahl@xxxxxxxxx> >>> --- >>> drivers/net/wireless/ath/ath10k/sdio.c | 7 ++++++- >>> 1 file changed, 6 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/net/wireless/ath/ath10k/sdio.c b/drivers/net/wireless/ath/ath10k/sdio.c >>> index 3eb241cb8a25..989e3f563f3d 100644 >>> --- a/drivers/net/wireless/ath/ath10k/sdio.c >>> +++ b/drivers/net/wireless/ath/ath10k/sdio.c >>> @@ -1496,7 +1496,12 @@ static int ath10k_sdio_hif_tx_sg(struct ath10k *ar, u8 pipe_id, >>> skb = items[i].transfer_context; >>> padded_len = ath10k_sdio_calc_txrx_padded_len(ar_sdio, >>> skb->len); >>> - skb_trim(skb, padded_len); >>> + /* FIXME: unsure if just extending the skb len is the right >>> + * thing to do since we might read outside the skb->data >>> + * buffer. But we really don't want to realloc the skb just to >>> + * pad the length. >>> + */ >>> + skb->len = padded_len; >> >> Good catch! But I don't think you can modify skb->len directly like >> that. There is skb_pad() but that doesn't change skb->len, so that most >> likely needs more changes. So maybe skb_put() is the safest here? >> > I have tried a few different solutions for this, but none seems to be > bullet proof. > > skb_pad() raises a BUG() if there is not enough space in skb->data. > > The best candidate so far has been skb_put_padto(). It pads and reallocates > the skb if needed. > > The problem is that it also cause a panic if there is more than one reference > to the skb (skb_shared() returns true). > > Some of the management frames via nl80211 have a refcount of 2. > In this case it is not possible to free and allocate the skb since there are > other users/references. > > I think I will have to make some kind of solution where I copy the content of > the skb to an internal buffer instead. Sorry for the late reply, I see that you also tried a different (and more complex) approach here: https://patchwork.kernel.org/patch/10906011/ In my opinion the cleanest approach would be to add extra_tx_tailroom to struct ieee80211_hw, similarly like we have extra_tx_headroom, and that way ath10k could easily add the padding with skb_pad(). Or what do you think? Of course I don't know what Johannes thinks as it would need several changes in mac80211, but at least struct net_device has needed_tailroom as well. And I would imagine that there is some other hardware which needs to do padding like this, or are ath10k SDIO devices really the first mac80211 drivers needing tailroom? -- https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
