diff mbox series

[net-next,v4,4/5] net: hdlc_fr: Do skb_reset_mac_header for skbs received on normal PVC devices

Message ID 20201030022839.438135-5-xie.he.0141@gmail.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series net: hdlc_fr: Improve fr_rx and add support for any Ethertype | expand

Commit Message

Xie He Oct. 30, 2020, 2:28 a.m. UTC
When an skb is received on a normal (non-Ethernet-emulating) PVC device,
call skb_reset_mac_header before we pass it to upper layers.

This is because normal PVC devices don't have header_ops, so any header we
have would not be visible to upper layer code when sending, so the header
shouldn't be visible to upper layer code when receiving, either.

Cc: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Cc: Krzysztof Halasa <khc@pm.waw.pl>
Signed-off-by: Xie He <xie.he.0141@gmail.com>
---
 drivers/net/wan/hdlc_fr.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Willem de Bruijn Oct. 30, 2020, 4:29 p.m. UTC | #1
On Thu, Oct 29, 2020 at 10:33 PM Xie He <xie.he.0141@gmail.com> wrote:
>
> When an skb is received on a normal (non-Ethernet-emulating) PVC device,
> call skb_reset_mac_header before we pass it to upper layers.
>
> This is because normal PVC devices don't have header_ops, so any header we
> have would not be visible to upper layer code when sending, so the header
> shouldn't be visible to upper layer code when receiving, either.
>
> Cc: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> Cc: Krzysztof Halasa <khc@pm.waw.pl>
> Signed-off-by: Xie He <xie.he.0141@gmail.com>

Acked-by: Willem de Bruijn <willemb@google.com>

Should this go to net if a bugfix though?
Xie He Oct. 30, 2020, 8:08 p.m. UTC | #2
On Fri, Oct 30, 2020 at 9:30 AM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> Acked-by: Willem de Bruijn <willemb@google.com>

Thanks for your ack!

> Should this go to net if a bugfix though?

Yes, this should theoretically be a bug fix. But I didn't think this
was an important fix, because af_packet.c already had workarounds for
this issue for all drivers that didn't have header_ops. We can
separate this patch to make it go to "net" though, but I'm afraid that
this will cause merging conflicts between "net" and "net-next".
Willem de Bruijn Oct. 30, 2020, 9:28 p.m. UTC | #3
On Fri, Oct 30, 2020 at 4:08 PM Xie He <xie.he.0141@gmail.com> wrote:
>
> On Fri, Oct 30, 2020 at 9:30 AM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
> >
> > Acked-by: Willem de Bruijn <willemb@google.com>
>
> Thanks for your ack!
>
> > Should this go to net if a bugfix though?
>
> Yes, this should theoretically be a bug fix. But I didn't think this
> was an important fix, because af_packet.c already had workarounds for
> this issue for all drivers that didn't have header_ops. We can
> separate this patch to make it go to "net" though, but I'm afraid that
> this will cause merging conflicts between "net" and "net-next".

Yes, it might require holding off the other patches until net is
merged into net-next.

Packet sockets are likely not the only way these packets are received?
It changes mac_len as computed in __netif_receive_skb_core.

If there is no real bug that is fixed, net-next is fine.
Xie He Oct. 30, 2020, 9:49 p.m. UTC | #4
On Fri, Oct 30, 2020 at 2:28 PM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> Yes, it might require holding off the other patches until net is
> merged into net-next.
>
> Packet sockets are likely not the only way these packets are received?
> It changes mac_len as computed in __netif_receive_skb_core.

I looked at __netif_receive_skb_core. I didn't see it computing mac_len?

I thought only AF_PACKET/RAW sockets would need this information
because other upper layers would not care about what happened in L2.

I see mac_len is computed in netif_receive_generic_xdp. I'm not clear
about the reason why it calculates it. But it seems that it considers
the L2 header as an Ethernet header, which is incorrect for this
driver.

> If there is no real bug that is fixed, net-next is fine.
Willem de Bruijn Oct. 30, 2020, 10:22 p.m. UTC | #5
On Fri, Oct 30, 2020 at 5:49 PM Xie He <xie.he.0141@gmail.com> wrote:
>
> On Fri, Oct 30, 2020 at 2:28 PM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
> >
> > Yes, it might require holding off the other patches until net is
> > merged into net-next.
> >
> > Packet sockets are likely not the only way these packets are received?
> > It changes mac_len as computed in __netif_receive_skb_core.
>
> I looked at __netif_receive_skb_core. I didn't see it computing mac_len?

It's indirect:

        skb_reset_network_header(skb);
        if (!skb_transport_header_was_set(skb))
                skb_reset_transport_header(skb);
        skb_reset_mac_len(skb);

> I thought only AF_PACKET/RAW sockets would need this information
> because other upper layers would not care about what happened in L2.

I think that's a reasonable assumption. I don't have a good
counterexample ready. Specific to this case, it seems to have been
working with no one complaining so far ;)

> I see mac_len is computed in netif_receive_generic_xdp. I'm not clear
> about the reason why it calculates it. But it seems that it considers
> the L2 header as an Ethernet header, which is incorrect for this
> driver.
>
> > If there is no real bug that is fixed, net-next is fine.
Xie He Oct. 30, 2020, 10:52 p.m. UTC | #6
On Fri, Oct 30, 2020 at 3:22 PM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> It's indirect:
>
>         skb_reset_network_header(skb);
>         if (!skb_transport_header_was_set(skb))
>                 skb_reset_transport_header(skb);
>         skb_reset_mac_len(skb);

Oh. I see. skb_reset_mac_len would set skb->mac_len. Not sure where
skb->mac_len would be used though.

> > I thought only AF_PACKET/RAW sockets would need this information
> > because other upper layers would not care about what happened in L2.
>
> I think that's a reasonable assumption. I don't have a good
> counterexample ready. Specific to this case, it seems to have been
> working with no one complaining so far ;)

Yeah. It seems to me that a lot of drivers (without header_ops) have
this problem. The comment in af_packet.c before my commit b79a80bd6dd8
also indicated this problem was widespread. It seemed to not cause any
issues.
diff mbox series

Patch

diff --git a/drivers/net/wan/hdlc_fr.c b/drivers/net/wan/hdlc_fr.c
index 3639c2bfb141..9a37575686b9 100644
--- a/drivers/net/wan/hdlc_fr.c
+++ b/drivers/net/wan/hdlc_fr.c
@@ -935,6 +935,7 @@  static int fr_rx(struct sk_buff *skb)
 		skb_pull(skb, 4); /* Remove 4-byte header (hdr, UI, NLPID) */
 		skb->dev = pvc->main;
 		skb->protocol = htons(ETH_P_IP);
+		skb_reset_mac_header(skb);
 
 	} else if (data[3] == NLPID_IPV6) {
 		if (!pvc->main)
@@ -942,6 +943,7 @@  static int fr_rx(struct sk_buff *skb)
 		skb_pull(skb, 4); /* Remove 4-byte header (hdr, UI, NLPID) */
 		skb->dev = pvc->main;
 		skb->protocol = htons(ETH_P_IPV6);
+		skb_reset_mac_header(skb);
 
 	} else if (skb->len > 10 && data[3] == FR_PAD &&
 		   data[4] == NLPID_SNAP && data[5] == FR_PAD) {
@@ -958,6 +960,7 @@  static int fr_rx(struct sk_buff *skb)
 				goto rx_drop;
 			skb->dev = pvc->main;
 			skb->protocol = htons(pid);
+			skb_reset_mac_header(skb);
 			break;
 
 		case 0x80C20007: /* bridged Ethernet frame */