| Message ID | 20201120030412.1646940-1-eyal.birger@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net/packet: fix incoming receive for L3 devices without visible hard header | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net |
| netdev/subject_prefix | success | Link |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 2 this patch: 2 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | warning | CHECK: Comparison to NULL could be written "!dev->header_ops" CHECK: Comparison to NULL could be written "!dev->header_ops->create" CHECK: Comparison to NULL could be written "dev->header_ops" CHECK: Comparison to NULL could be written "dev->header_ops->create" |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 2 this patch: 2 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
On Thu, Nov 19, 2020 at 10:05 PM Eyal Birger <eyal.birger@gmail.com> wrote: > > In the patchset merged by commit b9fcf0a0d826 > ("Merge branch 'support-AF_PACKET-for-layer-3-devices'") L3 devices which > did not have header_ops were given one for the purpose of protocol parsing > on af_packet transmit path. > > That change made af_packet receive path regard these devices as having a > visible L3 header and therefore aligned incoming skb->data to point to the > skb's mac_header. Some devices, such as ipip, xfrmi, and others, do not > reset their mac_header prior to ingress and therefore their incoming > packets became malformed. > > Ideally these devices would reset their mac headers, or af_packet would be > able to rely on dev->hard_header_len being 0 for such cases, but it seems > this is not the case. > > Fix by changing af_packet RX ll visibility criteria to include the > existence of a '.create()' header operation, which is used when creating > a device hard header - via dev_hard_header() - by upper layers, and does > not exist in these L3 devices. > > Fixes: b9fcf0a0d826 ("Merge branch 'support-AF_PACKET-for-layer-3-devices'") > Signed-off-by: Eyal Birger <eyal.birger@gmail.com> Thanks for the fix. This makes sense to me. Recent discussions on this point also agreed that whether or not headers are exposed to code above the device driver depends on dev->hard_header_len and dev->header_ops->create (and the two have to be consistent with one another). dev->header_ops->parse_protocol is a best effort approach to infer a protocol in cases where the caller did not specify it. But as best effort, its existence or absence does not define the device header, so testing only header_ops != NULL is insufficient. > --- > net/packet/af_packet.c | 28 ++++++++++++++++++++-------- > 1 file changed, 20 insertions(+), 8 deletions(-) > > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c > index cefbd50c1090..a241059fd536 100644 > --- a/net/packet/af_packet.c > +++ b/net/packet/af_packet.c > @@ -93,8 +93,8 @@ > > /* > Assumptions: > - - If the device has no dev->header_ops, there is no LL header visible > - above the device. In this case, its hard_header_len should be 0. > + - If the device has no dev->header_ops->create, there is no LL header > + visible above the device. In this case, its hard_header_len should be 0. > The device may prepend its own header internally. In this case, its > needed_headroom should be set to the space needed for it to add its > internal header. > @@ -108,21 +108,21 @@ > On receive: > ----------- > > -Incoming, dev->header_ops != NULL > +Incoming, dev->header_ops != NULL && dev->header_ops->create != NULL > mac_header -> ll header > data -> data > > -Outgoing, dev->header_ops != NULL > +Outgoing, dev->header_ops != NULL && dev->header_ops->create != NULL > mac_header -> ll header > data -> ll header > > -Incoming, dev->header_ops == NULL > +Incoming, dev->header_ops == NULL || dev->header_ops->create == NULL > mac_header -> data > However drivers often make it point to the ll header. > This is incorrect because the ll header should be invisible to us. > data -> data > > -Outgoing, dev->header_ops == NULL > +Outgoing, dev->header_ops == NULL || dev->header_ops->create == NULL > mac_header -> data. ll header is invisible to us. > data -> data > > @@ -272,6 +272,18 @@ static bool packet_use_direct_xmit(const struct packet_sock *po) > return po->xmit == packet_direct_xmit; > } > > +static bool packet_ll_header_rcv_visible(const struct net_device *dev) > +{ > + /* The device has an explicit notion of ll header, > + * exported to higher levels > + * > + * Otherwise, the device hides details of its frame > + * structure, so that corresponding packet head is > + * never delivered to user. > + */ > + return dev->header_ops && dev->header_ops->create; > +} > + Perhaps a dev_has_header(..) in include/linux/netdevice.h And then the same in the Incoming/Outgoing comments above.
diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c index cefbd50c1090..a241059fd536 100644 --- a/net/packet/af_packet.c +++ b/net/packet/af_packet.c @@ -93,8 +93,8 @@ /* Assumptions: - - If the device has no dev->header_ops, there is no LL header visible - above the device. In this case, its hard_header_len should be 0. + - If the device has no dev->header_ops->create, there is no LL header + visible above the device. In this case, its hard_header_len should be 0. The device may prepend its own header internally. In this case, its needed_headroom should be set to the space needed for it to add its internal header. @@ -108,21 +108,21 @@ On receive: ----------- -Incoming, dev->header_ops != NULL +Incoming, dev->header_ops != NULL && dev->header_ops->create != NULL mac_header -> ll header data -> data -Outgoing, dev->header_ops != NULL +Outgoing, dev->header_ops != NULL && dev->header_ops->create != NULL mac_header -> ll header data -> ll header -Incoming, dev->header_ops == NULL +Incoming, dev->header_ops == NULL || dev->header_ops->create == NULL mac_header -> data However drivers often make it point to the ll header. This is incorrect because the ll header should be invisible to us. data -> data -Outgoing, dev->header_ops == NULL +Outgoing, dev->header_ops == NULL || dev->header_ops->create == NULL mac_header -> data. ll header is invisible to us. data -> data @@ -272,6 +272,18 @@ static bool packet_use_direct_xmit(const struct packet_sock *po) return po->xmit == packet_direct_xmit; } +static bool packet_ll_header_rcv_visible(const struct net_device *dev) +{ + /* The device has an explicit notion of ll header, + * exported to higher levels + * + * Otherwise, the device hides details of its frame + * structure, so that corresponding packet head is + * never delivered to user. + */ + return dev->header_ops && dev->header_ops->create; +} + static u16 packet_pick_tx_queue(struct sk_buff *skb) { struct net_device *dev = skb->dev; @@ -2069,7 +2081,7 @@ static int packet_rcv(struct sk_buff *skb, struct net_device *dev, skb->dev = dev; - if (dev->header_ops) { + if (packet_ll_header_rcv_visible(dev)) { /* The device has an explicit notion of ll header, * exported to higher levels. * @@ -2198,7 +2210,7 @@ static int tpacket_rcv(struct sk_buff *skb, struct net_device *dev, if (!net_eq(dev_net(dev), sock_net(sk))) goto drop; - if (dev->header_ops) { + if (packet_ll_header_rcv_visible(dev)) { if (sk->sk_type != SOCK_DGRAM) skb_push(skb, skb->data - skb_mac_header(skb)); else if (skb->pkt_type == PACKET_OUTGOING) {
In the patchset merged by commit b9fcf0a0d826 ("Merge branch 'support-AF_PACKET-for-layer-3-devices'") L3 devices which did not have header_ops were given one for the purpose of protocol parsing on af_packet transmit path. That change made af_packet receive path regard these devices as having a visible L3 header and therefore aligned incoming skb->data to point to the skb's mac_header. Some devices, such as ipip, xfrmi, and others, do not reset their mac_header prior to ingress and therefore their incoming packets became malformed. Ideally these devices would reset their mac headers, or af_packet would be able to rely on dev->hard_header_len being 0 for such cases, but it seems this is not the case. Fix by changing af_packet RX ll visibility criteria to include the existence of a '.create()' header operation, which is used when creating a device hard header - via dev_hard_header() - by upper layers, and does not exist in these L3 devices. Fixes: b9fcf0a0d826 ("Merge branch 'support-AF_PACKET-for-layer-3-devices'") Signed-off-by: Eyal Birger <eyal.birger@gmail.com> --- net/packet/af_packet.c | 28 ++++++++++++++++++++-------- 1 file changed, 20 insertions(+), 8 deletions(-)