DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org A39DA6155C
From:   Kalle Valo <kvalo@codeaurora.org>
To:     Erik Stromdahl <erik.stromdahl@gmail.com>
Cc:     linux-wireless@vger.kernel.org, ath10k@lists.infradead.org
Subject: Re: [PATCH 6/6] ath10k: sdio: replace skb_trim with explicit set of skb->len
References: <20190409190851.4557-1-erik.stromdahl@gmail.com>
        <20190409190851.4557-7-erik.stromdahl@gmail.com>
Date:   Fri, 12 Apr 2019 16:17:26 +0300
In-Reply-To: <20190409190851.4557-7-erik.stromdahl@gmail.com> (Erik
        Stromdahl's message of "Tue, 9 Apr 2019 21:08:51 +0200")
Message-ID: <87zhovcqhl.fsf@kamboji.qca.qualcomm.com>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

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?

-- 
Kalle Valo