| Message ID | 20190409190851.4557-7-erik.stromdahl@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | ath10k: SDIO and high latency patches from Silex | expand |
Erik Stromdahl <erik.stromdahl@gmail.com> 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@gmail.com> > --- > 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?
On 4/12/19 3:17 PM, Kalle Valo wrote: > Erik Stromdahl <erik.stromdahl@gmail.com> 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@gmail.com> >> --- >> 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.
+ johannes Erik Stromdahl <erik.stromdahl@gmail.com> writes: > On 4/12/19 3:17 PM, Kalle Valo wrote: >> Erik Stromdahl <erik.stromdahl@gmail.com> 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@gmail.com> >>> --- >>> 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?
On Tue, 2019-10-01 at 15:21 +0300, Kalle Valo wrote: > > > > 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? This seems unsafe to me - if you don't have any tailroom, then you'll end up sending data to the device that's not really for the device, or depending on how all this is allocated you might even fault later because of sdio_memcpy_toio(..., ..., skb->data, skb->len)... > > 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. As it should. > > 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). As it also should :-) > 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? I disagree, adding tailroom to the SKB just for padding would be useless... Probably all you really have to do is this: --- a/drivers/net/wireless/ath/ath10k/sdio.c +++ b/drivers/net/wireless/ath/ath10k/sdio.c @@ -1485,11 +1485,10 @@ 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); /* Write TX data to the end of the mbox address space */ address = ar_sdio->mbox_addr[eid] + ar_sdio->mbox_size[eid] - - skb->len; + padded_len; ret = ath10k_sdio_prep_async_req(ar, address, skb, NULL, true, eid); if (ret) since the device evidently doesn't care what's in the pad bytes, so it can just stay as is inside its own memory? johannes
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; /* Write TX data to the end of the mbox address space */ address = ar_sdio->mbox_addr[eid] + ar_sdio->mbox_size[eid] -
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@gmail.com> --- drivers/net/wireless/ath/ath10k/sdio.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)