MIME-Version: 1.0
References: <20230319220954.21834-1-josef@miegl.cz>
In-Reply-To: <20230319220954.21834-1-josef@miegl.cz>
From:   Eyal Birger <eyal.birger@gmail.com>
Date:   Tue, 21 Mar 2023 11:59:02 +0200
Message-ID: <CAHsH6GugMRzxJRpwK6F6hWXRJ3-o3kqXrnvjXSWZqSLBGv9_VA@mail.gmail.com>
Subject: Re: [PATCH net-next v2] net: geneve: accept every ethertype
To:     Josef Miegl <josef@miegl.cz>
Cc:     Pravin B Shelar <pshelar@ovn.org>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Mon, Mar 20, 2023 at 12:10=E2=80=AFAM Josef Miegl <josef@miegl.cz> wrote=
:
>
> The Geneve encapsulation, as defined in RFC 8926, has a Protocol Type
> field, which states the Ethertype of the payload appearing after the
> Geneve header.
>
> Commit 435fe1c0c1f7 ("net: geneve: support IPv4/IPv6 as inner protocol")
> introduced a new IFLA_GENEVE_INNER_PROTO_INHERIT flag that allowed the
> use of other Ethertypes than Ethernet. However, it did not get rid of a
> restriction that prohibits receiving payloads other than Ethernet,
> instead the commit white-listed additional Ethertypes, IPv4 and IPv6.
>
> This patch removes this restriction, making it possible to receive any
> Ethertype as a payload, if the IFLA_GENEVE_INNER_PROTO_INHERIT flag is
> set.
>
> The restriction was set in place back in commit 0b5e8b8eeae4
> ("net: Add Geneve tunneling protocol driver"), which implemented a
> protocol layer driver for Geneve to be used with Open vSwitch. The
> relevant discussion about introducing the Ethertype white-list can be
> found here:
> https://lore.kernel.org/netdev/CAEP_g=3D_1q3ACX5NTHxLDnysL+dTMUVzdLpgw1ap=
LKEdDSWPztw@mail.gmail.com/
>
> <quote>
> >> +       if (unlikely(geneveh->proto_type !=3D htons(ETH_P_TEB)))
> >
> > Why? I thought the point of geneve carrying protocol field was to
> > allow protocols other than Ethernet... is this temporary maybe?
>
> Yes, it is temporary. Currently OVS only handles Ethernet packets but
> this restriction can be lifted once we have a consumer that is capable
> of handling other protocols.
> </quote>
>
> This white-list was then ported to a generic Geneve netdevice in commit
> 371bd1061d29 ("geneve: Consolidate Geneve functionality in single
> module."). Preserving the Ethertype white-list at this point made sense,
> as the Geneve device could send out only Ethernet payloads anyways.
>
> However, now that the Geneve netdevice supports encapsulating other
> payloads with IFLA_GENEVE_INNER_PROTO_INHERIT and we have a consumer
> capable of other protocols, it seems appropriate to lift the restriction
> and allow any Geneve payload to be received.
>
> Signed-off-by: Josef Miegl <josef@miegl.cz>
> ---
>  drivers/net/geneve.c | 9 ++-------
>  1 file changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
> index 89ff7f8e8c7e..32684e94eb4f 100644
> --- a/drivers/net/geneve.c
> +++ b/drivers/net/geneve.c
> @@ -365,13 +365,6 @@ static int geneve_udp_encap_recv(struct sock *sk, st=
ruct sk_buff *skb)
>         if (unlikely(geneveh->ver !=3D GENEVE_VER))
>                 goto drop;
>
> -       inner_proto =3D geneveh->proto_type;
> -
> -       if (unlikely((inner_proto !=3D htons(ETH_P_TEB) &&
> -                     inner_proto !=3D htons(ETH_P_IP) &&
> -                     inner_proto !=3D htons(ETH_P_IPV6))))
> -               goto drop;
> -
>         gs =3D rcu_dereference_sk_user_data(sk);
>         if (!gs)
>                 goto drop;
> @@ -380,6 +373,8 @@ static int geneve_udp_encap_recv(struct sock *sk, str=
uct sk_buff *skb)
>         if (!geneve)
>                 goto drop;
>
> +       inner_proto =3D geneveh->proto_type;
> +
>         if (unlikely((!geneve->cfg.inner_proto_inherit &&
>                       inner_proto !=3D htons(ETH_P_TEB)))) {
>                 geneve->dev->stats.rx_dropped++;
> --
> 2.37.1
>

Reviewed-by: Eyal Birger <eyal.birger@gmail.com>