linux-kernel - Re: [PATCH net-next] net: ipa: pass checksum trailer with received packets

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <3f3a5630-f4af-1dec-be9f-2b3868ad60bf@linaro.org>
Date:   Thu, 11 Feb 2021 05:25:32 -0600
From:   Alex Elder <elder@...aro.org>
To:     davem@...emloft.net, kuba@...nel.org
Cc:     elder@...nel.org, evgreen@...omium.org, bjorn.andersson@...aro.org,
        cpratapa@...eaurora.org, subashab@...eaurora.org,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next] net: ipa: pass checksum trailer with received
 packets

On 2/10/21 3:13 PM, Alex Elder wrote:
> For a QMAP RX endpoint, received packets will be passed to the RMNet
> driver.  If RX checksum offload is enabled, the RMNet driver expects
> to find a trailer following each packet that contains computed
> checksum information.  Currently the IPA driver is passing the
> packet without the trailer.
> 
> Fix this bug.
> 
> Fixes: 84f9bd12d46db ("soc: qcom: ipa: IPA endpoints")
> Signed-off-by: Alex Elder <elder@...aro.org>

I want to give this patch a little more thought.

In the end it's not that critical (this is not
in the normal RX completion data path), and
the way things are currently configured we
won't have checksum offload enabled for the
receiving endpoint anyway.

     --> So I WITHDRAW this patch. <--

I don't think the patch is wrong, but I'm going
to avoid the backport hassle, and wait to address
the issue until it really matters.

Thanks.

					-Alex

> ---
> 
> David/Jakub,
> I would like to have this back-ported as bug fix.  At its core, the
> fix is simple, but even if it were reduced to a one-line change, the
> result won't cleanly apply to both net/master and net-next/master.
> How should this be handled?  What can I do to make it easier?
> 
> Thanks.
> 
> 					-Alex
> 
>   drivers/net/ipa/ipa_endpoint.c | 15 ++++++++++-----
>   1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ipa/ipa_endpoint.c b/drivers/net/ipa/ipa_endpoint.c
> index 7209ee3c31244..5e3c2b3f38a95 100644
> --- a/drivers/net/ipa/ipa_endpoint.c
> +++ b/drivers/net/ipa/ipa_endpoint.c
> @@ -1232,6 +1232,11 @@ static void ipa_endpoint_status_parse(struct ipa_endpoint *endpoint,
>   	void *data = page_address(page) + NET_SKB_PAD;
>   	u32 unused = IPA_RX_BUFFER_SIZE - total_len;
>   	u32 resid = total_len;
> +	u32 trailer_len = 0;
> +
> +	/* If checksum offload is enabled, each packet includes a trailer */
> +	if (endpoint->data->checksum)
> +		trailer_len = sizeof(struct rmnet_map_dl_csum_trailer);
>   
>   	while (resid) {
>   		const struct ipa_status *status = data;
> @@ -1260,18 +1265,18 @@ static void ipa_endpoint_status_parse(struct ipa_endpoint *endpoint,
>   		 */
>   		align = endpoint->data->rx.pad_align ? : 1;
>   		len = le16_to_cpu(status->pkt_len);
> -		len = sizeof(*status) + ALIGN(len, align);
> -		if (endpoint->data->checksum)
> -			len += sizeof(struct rmnet_map_dl_csum_trailer);
> +		len = sizeof(*status) + ALIGN(len, align) + trailer_len;
>   
>   		if (!ipa_endpoint_status_drop(endpoint, status)) {
>   			void *data2;
>   			u32 extra;
>   			u32 len2;
>   
> -			/* Client receives only packet data (no status) */
> +			/* Strip off the status element and pass only the
> +			 * packet data (plus checksum trailer if enabled).
> +			 */
>   			data2 = data + sizeof(*status);
> -			len2 = le16_to_cpu(status->pkt_len);
> +			len2 = le16_to_cpu(status->pkt_len) + trailer_len;
>   
>   			/* Have the true size reflect the extra unused space in
>   			 * the original receive buffer.  Distribute the "cost"
>