| 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: <6961baad-c64d-22e5-4425-adb33b9b6236@ieee.org>
Date: Fri, 23 Apr 2021 07:32:06 -0500
From: Alex Elder <elder@...e.org>
To: Sharath Chandra Vurukala <sharathv@...eaurora.org>,
davem@...emloft.net, kuba@...nel.org, elder@...nel.org,
cpratapa@...eaurora.org, subashab@...eaurora.org,
netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next v4 2/3] net: ethernet: rmnet: Support for ingress
MAPv5 checksum offload
On 4/22/21 3:02 PM, 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.
>
> 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>
I have a few minor things I recommend you fix.
I don't see any bugs, but I'm not going to offer
"Reviewed-by" until you've had a chance to either
update your patch or explain why you won't.
> ---
> .../net/ethernet/qualcomm/rmnet/rmnet_handlers.c | 17 ++++---
> drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h | 3 +-
> .../net/ethernet/qualcomm/rmnet/rmnet_map_data.c | 58 +++++++++++++++++++++-
> include/linux/if_rmnet.h | 27 +++++++++-
> include/uapi/linux/if_link.h | 1 +
> 5 files changed, 97 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
> index 0be5ac7..706a225 100644
> --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
> +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
> @@ -1,5 +1,5 @@
> // SPDX-License-Identifier: GPL-2.0-only
> -/* Copyright (c) 2013-2018, The Linux Foundation. All rights reserved.
> +/* Copyright (c) 2013-2018, 2021, The Linux Foundation. All rights reserved.
> *
> * RMNET Data ingress/egress handler
> */
> @@ -82,11 +82,16 @@ __rmnet_map_ingress_handler(struct sk_buff *skb,
>
> skb->dev = ep->egress_dev;
>
> - /* Subtract MAP header */
> - skb_pull(skb, sizeof(struct rmnet_map_header));
> - rmnet_set_skb_proto(skb);
> -
> - if (port->data_format & RMNET_FLAGS_INGRESS_MAP_CKSUMV4) {
> + if ((port->data_format & RMNET_FLAGS_INGRESS_MAP_CKSUMV5) &&
> + (map_header->flags & MAP_NEXT_HEADER_FLAG)) {
> + if (rmnet_map_process_next_hdr_packet(skb, len))
> + goto free_skb;
> + skb_pull(skb, sizeof(*map_header));
> + rmnet_set_skb_proto(skb);
> + } else if (port->data_format & RMNET_FLAGS_INGRESS_MAP_CKSUMV4) {
> + /* Subtract MAP header */
> + skb_pull(skb, sizeof(*map_header));
> + rmnet_set_skb_proto(skb);
> if (!rmnet_map_checksum_downlink_packet(skb, len + pad))
> skb->ip_summed = CHECKSUM_UNNECESSARY;
> }
> diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h
> index 2aea153..1a399bf 100644
> --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h
> +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h
> @@ -1,5 +1,5 @@
> /* SPDX-License-Identifier: GPL-2.0-only */
> -/* Copyright (c) 2013-2018, The Linux Foundation. All rights reserved.
> +/* Copyright (c) 2013-2018, 2021, The Linux Foundation. All rights reserved.
> */
>
> #ifndef _RMNET_MAP_H_
> @@ -48,5 +48,6 @@ void rmnet_map_command(struct sk_buff *skb, struct rmnet_port *port);
> int rmnet_map_checksum_downlink_packet(struct sk_buff *skb, u16 len);
> void rmnet_map_checksum_uplink_packet(struct sk_buff *skb,
> struct net_device *orig_dev);
> +int rmnet_map_process_next_hdr_packet(struct sk_buff *skb, u16 len);
>
> #endif /* _RMNET_MAP_H_ */
> diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
> index 0ac2ff8..f427018 100644
> --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
> +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
> @@ -1,5 +1,5 @@
> // SPDX-License-Identifier: GPL-2.0-only
> -/* Copyright (c) 2013-2018, The Linux Foundation. All rights reserved.
> +/* Copyright (c) 2013-2018, 2021, The Linux Foundation. All rights reserved.
> *
> * RMNET Data MAP protocol
> */
> @@ -8,6 +8,7 @@
> #include <linux/ip.h>
> #include <linux/ipv6.h>
> #include <net/ip6_checksum.h>
> +#include <linux/bitfield.h>
> #include "rmnet_config.h"
> #include "rmnet_map.h"
> #include "rmnet_private.h"
> @@ -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;
> + unsigned char *data = skb->data;
If you define the data variable to have (void *) type you can
get rid of some casts below, and it won't need to use a cast
when assigned here either.
> struct rmnet_map_header *maph;
> struct sk_buff *skbn;
> + u8 nexthdr_type;
> u32 packet_len;
>
> if (skb->len == 0)
> @@ -312,6 +316,17 @@ struct sk_buff *rmnet_map_deaggregate(struct sk_buff *skb,
>
> if (port->data_format & RMNET_FLAGS_INGRESS_MAP_CKSUMV4)
> packet_len += sizeof(struct rmnet_map_dl_csum_trailer);
This block should be surrounded by curly braces. If one block
in an if statement (or chain of them) has them, they all should.
> + else if (port->data_format & RMNET_FLAGS_INGRESS_MAP_CKSUMV5) {
> + if (!(maph->flags & MAP_CMD_FLAG)) {
> + packet_len += sizeof(struct rmnet_map_v5_csum_header);
packet_len += sizeof(*next_hdr);
> + if (maph->flags & MAP_NEXT_HEADER_FLAG)
> + next_hdr = (struct rmnet_map_v5_csum_header *)
> + (data + sizeof(*maph));
If data is a void pointer, this could be:
next_hdr = data + sizeof(*maph);
> + else
> + /* Mapv5 data pkt without csum hdr is invalid */
> + return NULL;
> + }
> + }
>
> if (((int)skb->len - (int)packet_len) < 0)
> return NULL;
> @@ -320,6 +335,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);
Consider fixing the alignment of the second argument
above.
> + 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 +436,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;
> +
> + 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);
The arguments in the above two assignments don't look
aligned very well. Maybe it's just the way my mail
program is displaying it.
> +
> + 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(struct rmnet_map_v5_csum_header));
skb_pull(skb, sizeof(*next_hdr);
> + } else
> + return -EINVAL;
Please add curly braces here too.
> +
> + return rc;
> +}
> +
> diff --git a/include/linux/if_rmnet.h b/include/linux/if_rmnet.h
> index 4efb537..f82e37e 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
> */
> #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,27 @@ 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);
> +
> +/* v5 header_info field
> + * NEXT_HEADER: Represents whether there is any other header
> + * HEADER TYPE: represents the type of this header
> + *
> + * 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_SHIFT 1
> +#define MAPV5_HDRINFO_HDR_TYPE_FMASK GENMASK(7, MAPV5_HDRINFO_HDR_TYPE_SHIFT)
This is the only place you use MAPV5_HDRINFO_TYPE_SHIFT.
Defining and using it here immediately after its definition
subtracts, rather than adds value. Just do:
#define MAPV5_HDRINFO_HDR_TYPE_FMASK GENMASK(7, 1)
-Alex
> +#define MAPV5_CSUMINFO_VALID_FLAG BIT(7)
> +
> +#define RMNET_MAP_HEADER_TYPE_CSUM_OFFLOAD 2
> #endif /* !(_LINUX_IF_RMNET_H_) */
> diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
> index 91c8dda..21529b3 100644
> --- a/include/uapi/linux/if_link.h
> +++ b/include/uapi/linux/if_link.h
> @@ -1235,6 +1235,7 @@ enum {
> #define RMNET_FLAGS_INGRESS_MAP_COMMANDS (1U << 1)
> #define RMNET_FLAGS_INGRESS_MAP_CKSUMV4 (1U << 2)
> #define RMNET_FLAGS_EGRESS_MAP_CKSUMV4 (1U << 3)
> +#define RMNET_FLAGS_INGRESS_MAP_CKSUMV5 (1U << 4)
>
> enum {
> IFLA_RMNET_UNSPEC,
>
Powered by blists - more mailing lists