Received-SPF: pass (google.com: domain of linux-kernel+bounces-192542-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240527074456.9310-1-chengen.du@canonical.com>
 <66549c368764b_268e8229462@willemb.c.googlers.com.notmuch> <CAPza5qe-H6piY6ED7StLOiviiMbWq1rnMpKR_dZu1sehwhji2w@mail.gmail.com>
In-Reply-To: <CAPza5qe-H6piY6ED7StLOiviiMbWq1rnMpKR_dZu1sehwhji2w@mail.gmail.com>
From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Date: Tue, 28 May 2024 10:29:07 -0400
Message-ID: <CAF=yD-J8UV+KD7fUQ-eSJWvHrhqezMs81zXX=VeVgdHR8ZZ7ag@mail.gmail.com>
Subject: Re: [PATCH v3] af_packet: Handle outgoing VLAN packets without
 hardware offloading
To: Chengen Du <chengen.du@canonical.com>
Cc: davem@davemloft.net, edumazet@google.com, kuba@kernel.org, 
	pabeni@redhat.com, loke.chetan@gmail.com, netdev@vger.kernel.org, 
	linux-kernel@vger.kernel.org, stable@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Mon, May 27, 2024 at 11:40=E2=80=AFPM Chengen Du <chengen.du@canonical.c=
om> wrote:
>
> Hi Willem,
>
> Thank you for your suggestions on the patch.
> However, there are some parts I am not familiar with, and I would appreci=
ate more detailed information from your side.

Please respond with plain-text email. This message did not make it to
the list. Also no top posting.

https://docs.kernel.org/process/submitting-patches.html
https://subspace.kernel.org/etiquette.html

> > > @@ -2457,7 +2465,8 @@ static int tpacket_rcv(struct sk_buff *skb, str=
uct net_device *dev,
> > >       sll->sll_halen =3D dev_parse_header(skb, sll->sll_addr);
> > >       sll->sll_family =3D AF_PACKET;
> > >       sll->sll_hatype =3D dev->type;
> > > -     sll->sll_protocol =3D skb->protocol;
> > > +     sll->sll_protocol =3D (skb->protocol =3D=3D htons(ETH_P_8021Q))=
 ?
> > > +             vlan_eth_hdr(skb)->h_vlan_encapsulated_proto : skb->pro=
tocol;
> >
> > In SOCK_RAW mode, the VLAN tag will be present, so should be returned.
>
> Based on libpcap's handling, the SLL may not be used in SOCK_RAW mode.

The kernel fills in the sockaddr_ll fields in tpacket_rcv for both
SOCK_RAW and SOCK_DGRAM. Libpcap already can use both SOCK_RAW and
SOCK_DGRAM. And constructs the sll2_header pseudo header that tcpdump
sees itself, in pcap_handle_packet_mmap.

> Do you recommend evaluating the mode and maintaining the original logic i=
n SOCK_RAW mode,
> or should we use the same logic for both SOCK_DGRAM and SOCK_RAW modes?

I suggest keeping as is for SOCK_RAW, as returning data that starts at
a VLAN header together with skb->protocol of ETH_P_IPV6 would be just
as confusing as the inverse that we do today on SOCK_DGRAM.

> >
> > I'm concerned about returning a different value between SOCK_RAW and
> > SOCK_DGRAM. But don't immediately see a better option. And for
> > SOCK_DGRAM this approach is indistinguishable from the result on a
> > device with hardware offload, so is acceptable.
> >
> > This test for ETH_P_8021Q ignores the QinQ stacked VLAN case. When
> > fixing VLAN encap, both variants should be addressed at the same time.
> > Note that ETH_P_8021AD is included in the eth_type_vlan test you call
> > above.
>
> In patch 1, the eth_type_vlan() function is used to determine if we need =
to set the sll_protocol to the VLAN-encapsulated protocol, which includes b=
oth ETH_P_8021Q and ETH_P_8021AD.
> You mentioned previously that we might want the true network protocol ins=
tead of the inner VLAN tag in the QinQ case (which means 802.1ad?).
> I believe I may have misunderstood your point.

I mean that if SOCK_DGRAM strips all VLAN headers to return the data
from the start of the true network header, then skb->protocol should
return that network protocol.

With vlan stacking, your patch currently returns ETH_P_8021Q.

See the packet formats in
https://en.wikipedia.org/wiki/IEEE_802.1ad#Frame_format if you're
confused about how stacking works.

> Could you please confirm if both ETH_P_8021Q and ETH_P_8021AD should use =
the VLAN-encapsulated protocol when VLAN hardware offloading is unavailable=
?
> Or are there other aspects that this judgment does not handle correctly?