| Message ID | 20201030022839.438135-5-xie.he.0141@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: hdlc_fr: Improve fr_rx and add support for any Ethertype | expand |
| Context | Check | Description |
|---|---|---|
| jkicinski/cover_letter | success | Link |
| jkicinski/fixes_present | success | Link |
| jkicinski/patch_count | success | Link |
| jkicinski/tree_selection | success | Clearly marked for net-next |
| jkicinski/subject_prefix | success | Link |
| jkicinski/source_inline | success | Was 0 now: 0 |
| jkicinski/verify_signedoff | success | Link |
| jkicinski/module_param | success | Was 0 now: 0 |
| jkicinski/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| jkicinski/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| jkicinski/verify_fixes | success | Link |
| jkicinski/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 21 lines checked |
| jkicinski/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| jkicinski/header_inline | success | Link |
| jkicinski/stable | success | Stable not CCed |
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?
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".
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.
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.
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.
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 --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 */
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(+)