| 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
| ||
|
Message-ID: <915b733f-6312-94b3-099d-3c11eb3d3b32@ieee.org>
Date: Mon, 31 May 2021 09:34:02 -0500
From: Alex Elder <elder@...e.org>
To: Jakub Kicinski <kuba@...nel.org>,
Sharath Chandra Vurukala <sharathv@...eaurora.org>
Cc: davem@...emloft.net, elder@...nel.org, cpratapa@...eaurora.org,
netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next v7 2/3] net: ethernet: rmnet: Support for ingress
MAPv5 checksum offload
On 5/28/21 5:58 PM, Jakub Kicinski wrote:
> On Thu, 27 May 2021 14:18:41 +0530 Sharath Chandra Vurukala wrote:
>> Adding support for processing of MAPv5 downlink packets.
>> It involves parsing the Mapv5 packet and checking the csum header
>> to know whether the hardware has validated the checksum and is
>> valid or not.
Nice review Jakub. I will wait for version 8 and will review that.
-Alex
>>
>> Based on the checksum valid bit the corresponding stats are
>> incremented and skb->ip_summed is marked either CHECKSUM_UNNECESSARY
>> or left as CHEKSUM_NONE to let network stack revalidate the checksum
>> and update the respective snmp stats.
>>
>> Current MAPV1 header has been modified, the reserved field in the
>> Mapv1 header is now used for next header indication.
>>
>> Signed-off-by: Sharath Chandra Vurukala <sharathv@...eaurora.org>
>
>> @@ -300,8 +301,11 @@ struct rmnet_map_header *rmnet_map_add_map_header(struct sk_buff *skb,
>> struct sk_buff *rmnet_map_deaggregate(struct sk_buff *skb,
>> struct rmnet_port *port)
>> {
>> + struct rmnet_map_v5_csum_header *next_hdr = NULL;
>> + void *data = skb->data;
>> struct rmnet_map_header *maph;
>
> Please maintain reverse xmas tree ordering
>
>> struct sk_buff *skbn;
>> + u8 nexthdr_type;
>> u32 packet_len;
>>
>> if (skb->len == 0)
>> @@ -310,8 +314,18 @@ struct sk_buff *rmnet_map_deaggregate(struct sk_buff *skb,
>> maph = (struct rmnet_map_header *)skb->data;
>> packet_len = ntohs(maph->pkt_len) + sizeof(*maph);
>>
>> - if (port->data_format & RMNET_FLAGS_INGRESS_MAP_CKSUMV4)
>> + if (port->data_format & RMNET_FLAGS_INGRESS_MAP_CKSUMV4) {
>> packet_len += sizeof(struct rmnet_map_dl_csum_trailer);
>> + } else if (port->data_format & RMNET_FLAGS_INGRESS_MAP_CKSUMV5) {
>> + if (!(maph->flags & MAP_CMD_FLAG)) {
>> + packet_len += sizeof(*next_hdr);
>> + if (maph->flags & MAP_NEXT_HEADER_FLAG)
>> + next_hdr = (data + sizeof(*maph));
>
> brackets unnecessary
>
>> + else
>> + /* Mapv5 data pkt without csum hdr is invalid */
>> + return NULL;
>> + }
>> + }
>>
>> if (((int)skb->len - (int)packet_len) < 0)
>> return NULL;
>> @@ -320,6 +334,13 @@ struct sk_buff *rmnet_map_deaggregate(struct sk_buff *skb,
>> if (!maph->pkt_len)
>> return NULL;
>>
>> + if (next_hdr) {
>> + nexthdr_type = u8_get_bits(next_hdr->header_info,
>> + MAPV5_HDRINFO_HDR_TYPE_FMASK);
>> + if (nexthdr_type != RMNET_MAP_HEADER_TYPE_CSUM_OFFLOAD)
>> + return NULL;
>> + }
>> +
>> skbn = alloc_skb(packet_len + RMNET_MAP_DEAGGR_SPACING, GFP_ATOMIC);
>> if (!skbn)
>> return NULL;
>> @@ -414,3 +435,37 @@ void rmnet_map_checksum_uplink_packet(struct sk_buff *skb,
>>
>> priv->stats.csum_sw++;
>> }
>> +
>> +/* Process a MAPv5 packet header */
>> +int rmnet_map_process_next_hdr_packet(struct sk_buff *skb,
>> + u16 len)
>> +{
>> + struct rmnet_priv *priv = netdev_priv(skb->dev);
>> + struct rmnet_map_v5_csum_header *next_hdr;
>> + u8 nexthdr_type;
>> + int rc = 0;
>
> rc is not meaningfully used
>
>> + next_hdr = (struct rmnet_map_v5_csum_header *)(skb->data +
>> + sizeof(struct rmnet_map_header));
>> +
>> + nexthdr_type = u8_get_bits(next_hdr->header_info,
>> + MAPV5_HDRINFO_HDR_TYPE_FMASK);
>> +
>> + if (nexthdr_type == RMNET_MAP_HEADER_TYPE_CSUM_OFFLOAD) {
>> + if (unlikely(!(skb->dev->features & NETIF_F_RXCSUM))) {
>> + priv->stats.csum_sw++;
>> + } else if (next_hdr->csum_info & MAPV5_CSUMINFO_VALID_FLAG) {
>> + priv->stats.csum_ok++;
>> + skb->ip_summed = CHECKSUM_UNNECESSARY;
>> + } else {
>> + priv->stats.csum_valid_unset++;
>> + }
>> +
>> + /* Pull csum v5 header */
>> + skb_pull(skb, sizeof(*next_hdr));
>> + } else {
>> + return -EINVAL;
>
> flip condition, return early
>
>> + }
>> +
>> + return rc;
>> +}
>> diff --git a/include/linux/if_rmnet.h b/include/linux/if_rmnet.h
>> index 4efb537..8502ccc 100644
>> --- a/include/linux/if_rmnet.h
>> +++ b/include/linux/if_rmnet.h
>> @@ -1,5 +1,5 @@
>> /* SPDX-License-Identifier: GPL-2.0-only
>> - * Copyright (c) 2013-2019, The Linux Foundation. All rights reserved.
>> + * Copyright (c) 2013-2019, 2021 The Linux Foundation. All rights reserved.
>> */
>>
>> #ifndef _LINUX_IF_RMNET_H_
>> @@ -14,8 +14,10 @@ struct rmnet_map_header {
>> /* rmnet_map_header flags field:
>> * PAD_LEN: number of pad bytes following packet data
>> * CMD: 1 = packet contains a MAP command; 0 = packet contains data
>> + * NEXT_HEADER 1 = packet contains V5 CSUM header 0 = no V5 CSUM header
>
> Colon missing?
>
>> */
>> #define MAP_PAD_LEN_MASK GENMASK(5, 0)
>> +#define MAP_NEXT_HEADER_FLAG BIT(6)
>> #define MAP_CMD_FLAG BIT(7)
>>
>> struct rmnet_map_dl_csum_trailer {
>> @@ -45,4 +47,26 @@ struct rmnet_map_ul_csum_header {
>> #define MAP_CSUM_UL_UDP_FLAG BIT(14)
>> #define MAP_CSUM_UL_ENABLED_FLAG BIT(15)
>>
>> +/* MAP CSUM headers */
>> +struct rmnet_map_v5_csum_header {
>> + u8 header_info;
>> + u8 csum_info;
>> + __be16 reserved;
>> +} __aligned(1);
>
> __aligned() seems rather pointless here but ok.
>
>> +/* v5 header_info field
>> + * NEXT_HEADER: Represents whether there is any other header
>
> double space
>
>> + * HEADER TYPE: represents the type of this header
>
> On previous line you used _ for a space, and started from capital
> letter. Please be consistent.
>
>> + *
>> + * csum_info field
>> + * CSUM_VALID_OR_REQ:
>> + * 1 = for UL, checksum computation is requested.
>> + * 1 = for DL, validated the checksum and has found it valid
>> + */
>> +
>> +#define MAPV5_HDRINFO_NXT_HDR_FLAG BIT(0)
>> +#define MAPV5_HDRINFO_HDR_TYPE_FMASK GENMASK(7, 1)
>> +#define MAPV5_CSUMINFO_VALID_FLAG BIT(7)
>> +
>> +#define RMNET_MAP_HEADER_TYPE_CSUM_OFFLOAD 2
>> #endif /* !(_LINUX_IF_RMNET_H_) */
Powered by blists - more mailing lists