| 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: <ZH2plrPDtUdmjL7w@corigine.com>
Date: Mon, 5 Jun 2023 11:23:34 +0200
From: Simon Horman <simon.horman@...igine.com>
To: Marcin Szycik <marcin.szycik@...ux.intel.com>
Cc: intel-wired-lan@...ts.osuosl.org, netdev@...r.kernel.org,
wojciech.drewek@...el.com, michal.swiatkowski@...ux.intel.com,
alexandr.lobakin@...el.com, davem@...emloft.net, kuba@...nel.org,
jiri@...nulli.us, pabeni@...hat.com, jesse.brandeburg@...el.com,
idosch@...dia.com
Subject: Re: [RFC PATCH iwl-next 2/6] ip_tunnel: convert __be16 tunnel flags
to bitmaps
On Thu, Jun 01, 2023 at 03:19:25PM +0200, Marcin Szycik wrote:
> From: Alexander Lobakin <alexandr.lobakin@...el.com>
>
> Historically, tunnel flags like TUNNEL_CSUM or TUNNEL_ERSPAN_OPT
> have been defined as __be16. Now all of those 16 bits are occupied
> and there's no more free space for new flags.
> It can't be simply switched to a bigger container with no
> adjustments to the values, since it's an explicit Endian storage,
> and on LE systems (__be16)0x0001 equals to
> (__be64)0x0001000000000000.
> We could probably define new 64-bit flags depending on the
> Endianness, i.e. (__be64)0x0001 on BE and (__be64)0x00010000... on
> LE, but that would introduce an Endianness dependency and spawn a
> ton of Sparse warnings. To mitigate them, all of those places which
> were adjusted with this change would be touched anyway, so why not
> define stuff properly if there's no choice.
Hi Marcin,
a few nits from my side, that you can take or leave.
Overall this looks good to me.
Reviewed-by: Simon Horman <simon.horman@...igine.com>
> Define IP_TUNNEL_*_BIT counterparts as a bit number instead of the
> value already coded and a fistful of <16 <-> bitmap> converters and
> helpers. The two flags which have a different bit position are
> SIT_ISATAP_64 and VTI_ISVTI_64, as they were defined not as
> __cpu_to_be16(), but as (__force __be16), i.e. had different
> positions on LE and BE. Now they have a strongly defined place.
> Change all __be16 fields which were used to store those flags, to
> IP_TUNNEL_DECLARE_FLAGS() -> DECLARE_BITMAP(__IP_TUNNEL_FLAG_NUM) ->
> unsigned long[1] for now, and replace all TUNNEL_* occurencies to
> their 64-bit bitmap counterparts. Use the converters in the places
> which talk to the userspace, hardware (NFP) or other hosts (GRE
> header). The rest must explicitly use the new flags only. This must
> be done at once, otherwise there will be too much conversions
> throughout the code in the intermediate commits.
> Finally, disable the old __be16 flags for use in the kernel code
> (except for the two 'irregular' flags mentioned above), to prevent
> any accidential (mis)use of them. For the userspace, nothing is
nit: s/accidential/accidental/
> changed, only additions were made.
>
> Signed-off-by: Alexander Lobakin <alexandr.lobakin@...el.com>
> Signed-off-by: Marcin Szycik <marcin.szycik@...ux.intel.com>
> ---
> drivers/net/bareudp.c | 19 ++-
> .../ethernet/mellanox/mlx5/core/en/tc_tun.h | 2 +-
> .../mellanox/mlx5/core/en/tc_tun_encap.c | 6 +-
> .../mellanox/mlx5/core/en/tc_tun_geneve.c | 12 +-
> .../mellanox/mlx5/core/en/tc_tun_gre.c | 9 +-
> .../mellanox/mlx5/core/en/tc_tun_vxlan.c | 9 +-
> .../net/ethernet/mellanox/mlx5/core/en_tc.c | 15 +-
> .../ethernet/mellanox/mlxsw/spectrum_ipip.c | 32 +++--
> .../ethernet/mellanox/mlxsw/spectrum_span.c | 6 +-
> .../ethernet/netronome/nfp/flower/action.c | 26 +++-
> drivers/net/geneve.c | 46 +++---
> drivers/net/vxlan/vxlan_core.c | 14 +-
> include/net/dst_metadata.h | 10 +-
> include/net/flow_dissector.h | 2 +-
> include/net/gre.h | 59 ++++----
> include/net/ip6_tunnel.h | 4 +-
> include/net/ip_tunnels.h | 90 ++++++++++--
> include/net/udp_tunnel.h | 4 +-
> include/uapi/linux/if_tunnel.h | 33 +++++
> net/bridge/br_vlan_tunnel.c | 5 +-
> net/core/filter.c | 20 +--
> net/core/flow_dissector.c | 12 +-
> net/ipv4/fou_bpf.c | 2 +-
> net/ipv4/gre_demux.c | 2 +-
> net/ipv4/ip_gre.c | 131 +++++++++++-------
> net/ipv4/ip_tunnel.c | 51 ++++---
> net/ipv4/ip_tunnel_core.c | 81 +++++++----
> net/ipv4/ip_vti.c | 31 +++--
> net/ipv4/ipip.c | 21 ++-
> net/ipv4/udp_tunnel_core.c | 5 +-
> net/ipv6/ip6_gre.c | 87 +++++++-----
> net/ipv6/ip6_tunnel.c | 14 +-
> net/ipv6/sit.c | 9 +-
> net/netfilter/ipvs/ip_vs_core.c | 6 +-
> net/netfilter/ipvs/ip_vs_xmit.c | 20 +--
> net/netfilter/nft_tunnel.c | 45 +++---
> net/openvswitch/flow_netlink.c | 55 ++++----
> net/psample/psample.c | 26 ++--
> net/sched/act_tunnel_key.c | 39 +++---
> net/sched/cls_flower.c | 27 ++--
> 40 files changed, 695 insertions(+), 392 deletions(-)
nit: this is a rather long patch
...
> diff --git a/include/uapi/linux/if_tunnel.h b/include/uapi/linux/if_tunnel.h
> index 102119628ff5..d222e70d8621 100644
> --- a/include/uapi/linux/if_tunnel.h
> +++ b/include/uapi/linux/if_tunnel.h
> @@ -161,6 +161,14 @@ enum {
>
> #define IFLA_VTI_MAX (__IFLA_VTI_MAX - 1)
>
> +#ifndef __KERNEL__
> +/* Historically, tunnel flags have been defined as __be16 and now there are
> + * no free bits left. It is strongly advised to switch the already existing
> + * userspace code to u32/BIGINT and the new *_BIT definitions from down below,
> + * as __be16 can't be simply casted to a wider type on LE systems. All new
nit: s/casted/cast/
...
Powered by blists - more mailing lists