netdev - Re: [Intel-wired-lan] [PATCH iwl-next v8 09/14] libeth: move idpf_rx_csum_decoded and idpf_rx

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 for Android: free password hash cracker in your pocket

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

Message-ID: <086bf4f8-7a4c-4323-ae7a-c7c23223acec@intel.com>
Date: Tue, 30 Jul 2024 16:24:40 +0200
From: Alexander Lobakin <aleksander.lobakin@...el.com>
To: Mateusz Polchlopek <mateusz.polchlopek@...el.com>
CC: <intel-wired-lan@...ts.osuosl.org>, <netdev@...r.kernel.org>
Subject: Re: [Intel-wired-lan] [PATCH iwl-next v8 09/14] libeth: move
 idpf_rx_csum_decoded and idpf_rx_extracted

From: Mateusz Polchlopek <mateusz.polchlopek@...el.com>
Date: Tue, 30 Jul 2024 05:15:04 -0400

> Structs idpf_rx_csum_decoded and idpf_rx_extracted are used both in
> idpf and iavf Intel drivers. This commit changes the prefix from
> idpf_* to libeth_* and moves mentioned structs to libeth's rx.h header
> file.

[...]

> diff --git a/include/net/libeth/rx.h b/include/net/libeth/rx.h
> index 43574bd6612f..197ff0f5e34c 100644
> --- a/include/net/libeth/rx.h
> +++ b/include/net/libeth/rx.h
> @@ -258,4 +258,38 @@ static inline void libeth_rx_pt_set_hash(struct sk_buff *skb, u32 hash,
>  	skb_set_hash(skb, hash, pt.payload_layer);
>  }
>  
> +/**
> + * struct libeth_rx_csum_decoded - csum errors indicators

1. I'd name it just 'libeth_rx_csum'.
2. The brief you put at the end of the doc can be moved here.

 * struct libeth_rx_csum - Rx checksum offload status

> + * @l3l4p: L3 and L4 integrity check

What does it mean? kdoc should explain everything to help people understand.
>From this desc, I don't get whether it's a "valid checksum" status or
"invalid checksum" status. Whether it means that both L3 and L4 are
correct or can be only one of them?

> + * @ipe: IP checksum error indication

'indication' can be omitted. Just 'IP checksum error'.

> + * @eipe: External (most outer) IP header (only relevant for tunneled packets)

1. 'outermost'.
2. Please start each field's description from a lowercase letter.

 * @eipe: external

...

3. 'only relevant for tunneled packets' can be just 'only for tunnels'

> + * @eudpe: External (most outer) UDP checksum error (only relevant for tunneled
> + *         packets)
> + * @ipv6exadd: IPv6 with Destination Options Header or Routing Header

Maybe 'IPv6 header with extension headers'?

> + * @l4e: L4 integrity error indication

Same as for l3l4p.

> + *
> + * Checksum offload bits decoded from the receive descriptor.
> + */
> +struct libeth_rx_csum_decoded {
> +	u32 l3l4p:1;
> +	u32 ipe:1;
> +	u32 eipe:1;
> +	u32 eudpe:1;
> +	u32 ipv6exadd:1;
> +	u32 l4e:1;
> +	u32 pprs:1;
> +	u32 nat:1;
> +	u32 raw_csum_inv:1;
> +	u32 raw_csum:16;
> +};

1. Please place this struct (as well as the second one) after
   &libeth_rx_pt and align the indentation to it (my style is
   that I keep one indentation level per file).
3. You need a padding, so that ::raw_csum will be at offset of 2 bytes,
   i.e. aligned naturally. See how I align ::hash type in &libeth_rx_pt.
   I.e.

	u32					raw_csum_inv:1;

	u32					pad:7;
	u32					raw_csum:16;

4. I'd invert ::raw_csum_inv to ::raw_csum_valid.
5. kernel-doc for each field is needed. Here a half of fields doesn't
   have it.

scripts/kernel-doc -none -Wall include/net/libeth/rx.h

> +
> +struct libeth_rx_extracted {

The structure name says nothing.
Maybe

struct libeth_rqe_info {

"Receive queue element info".

I chose RQE, not "packet", as this info is per-descriptor, not
per-packet.

> +	unsigned int	size;

'len'?

> +	u16		vlan_tag;

Is it C-Tag or S-Tag or both depending on something or...?

> +	u16		rx_ptype;

Just 'ptype'?

> +	u8		end_of_packet:1;

Just 'eop'?

> +	u8		rxe:1;
> +};

1. kdoc. I've no idea what 'rxe' does mean.
2. Why `u8` and `u16`, but `unsigned int`, not `u32`?
   (I'd make everything u32:bits BTW)

3. You waste 4 bytes for only 2 bits.
   Since maximum ptype is currently 1024, while u16 can store up to
   65535, you could do

struct libeth_rqe_info {
	u32				len;
	u32				vlan_tag:16;

	u32				ptype:14;
	u32				eop:1;
	u32				rxe:1;
};

Most important: do you realize that not all of the fields are needed in
the same place, but some of them earlier, some later, and sometime not
even all of them are neede? For example, if it's not an EOP frag, you
would only need ::len and ::eop and that's it. The rest is only needed
for EOP fragments.
That means, reading all of them at once to one struct can lead to perf
regressions. One way to avoid this is to read-fields-when-needed, e.g.
always fill ::len and ::eop and the rest later if/when needed.

Also, can we just do a ptype lookup right away when we read ptype from
the descriptor and then pass &libeth_rx_pt everywhere, so that we won't
even need the ::ptype field here?

> +
> +
>  #endif /* __LIBETH_RX_H */

Thanks,
Olek