| Message ID | 20190114131841.1932-5-maximmi@mellanox.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | AF_PACKET transport_offset fix | expand |
On Mon, Jan 14, 2019 at 8:20 AM Maxim Mikityanskiy <maximmi@mellanox.com> wrote: > > If a socket was created with socket(AF_PACKET, SOCK_RAW, 0), the > protocol number is unavailable. Try to ask the driver to extract it from > the L2 header in order for skb_try_probe_transport_header to succeed. > > Signed-off-by: Maxim Mikityanskiy <maximmi@mellanox.com> > --- > net/packet/af_packet.c | 15 ++++++++++++--- > 1 file changed, 12 insertions(+), 3 deletions(-) > > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c > index 8fc76e68777a..d1d89749a17a 100644 > --- a/net/packet/af_packet.c > +++ b/net/packet/af_packet.c > @@ -1850,6 +1850,15 @@ static int packet_rcv_spkt(struct sk_buff *skb, struct net_device *dev, > return 0; > } > > +static void packet_parse_headers(struct sk_buff *skb, struct socket *sock) > +{ > + if (!skb->protocol && sock->type == SOCK_RAW) { > + skb_reset_mac_header(skb); > + skb->protocol = dev_parse_header_protocol(skb); > + } > + > + skb_try_probe_transport_header(skb); > +} In relation to the discussion at af_packet: fix raw sockets over 6in4 tunnel http://patchwork.ozlabs.org/patch/1023623/ if adding a new header_ops callback to parse link layer headers, please have it return both protocol and link layer header length. also, this information may be needed earlier in (t)packet_snd than the transport header, so I would suggest not combining the two in a new packet_parse_headers wrapper function.
On Mon, Jan 14, 2019 at 11:52 AM Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote: > > On Mon, Jan 14, 2019 at 8:20 AM Maxim Mikityanskiy <maximmi@mellanox.com> wrote: > > > > If a socket was created with socket(AF_PACKET, SOCK_RAW, 0), the > > protocol number is unavailable. Try to ask the driver to extract it from > > the L2 header in order for skb_try_probe_transport_header to succeed. > > > > Signed-off-by: Maxim Mikityanskiy <maximmi@mellanox.com> > > --- > > net/packet/af_packet.c | 15 ++++++++++++--- > > 1 file changed, 12 insertions(+), 3 deletions(-) > > > > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c > > index 8fc76e68777a..d1d89749a17a 100644 > > --- a/net/packet/af_packet.c > > +++ b/net/packet/af_packet.c > > @@ -1850,6 +1850,15 @@ static int packet_rcv_spkt(struct sk_buff *skb, struct net_device *dev, > > return 0; > > } > > > > +static void packet_parse_headers(struct sk_buff *skb, struct socket *sock) > > +{ > > + if (!skb->protocol && sock->type == SOCK_RAW) { > > + skb_reset_mac_header(skb); > > + skb->protocol = dev_parse_header_protocol(skb); > > + } > > + > > + skb_try_probe_transport_header(skb); > > +} > > > In relation to the discussion at > > af_packet: fix raw sockets over 6in4 tunnel > http://patchwork.ozlabs.org/patch/1023623/ > > if adding a new header_ops callback to parse link layer headers, > please have it return both protocol and link layer header length. This could just be an extension of existing header_ops->validate.
> > > +static void packet_parse_headers(struct sk_buff *skb, struct socket > *sock) > > > +{ > > > + if (!skb->protocol && sock->type == SOCK_RAW) { > > > + skb_reset_mac_header(skb); > > > + skb->protocol = dev_parse_header_protocol(skb); > > > + } > > > + > > > + skb_try_probe_transport_header(skb); > > > +} > > > > > > In relation to the discussion at > > > > af_packet: fix raw sockets over 6in4 tunnel > > http://patchwork.ozlabs.org/patch/1023623/ > > > > if adding a new header_ops callback to parse link layer headers, > > please have it return both protocol and link layer header length. Sorry, I miss the point here, can you elaborate more? If all you need is to have some header_ops callback that returns the L2 header length, there is one already, it's called parse. Or do you have a specific reason why you want my callback to also return the header length? > This could just be an extension of existing header_ops->validate. If you suggest extending an existing function, parse looks more suitable, but I decided not to touch the existing ones for two reasons: 1. I don't want to break the existing code that uses the parse function and will need to be modified to pass an extra parameter. 2. I don't want to spend machine time on copying the destination MAC when I only need the protocol, and vice versa. I'm looking forward to hearing your thoughts about it.
On Thu, Jan 17, 2019 at 4:10 AM Maxim Mikityanskiy <maximmi@mellanox.com> wrote: > > > > > +static void packet_parse_headers(struct sk_buff *skb, struct socket > > *sock) > > > > +{ > > > > + if (!skb->protocol && sock->type == SOCK_RAW) { > > > > + skb_reset_mac_header(skb); > > > > + skb->protocol = dev_parse_header_protocol(skb); > > > > + } > > > > + > > > > + skb_try_probe_transport_header(skb); > > > > +} > > > > > > > > > In relation to the discussion at > > > > > > af_packet: fix raw sockets over 6in4 tunnel > > > http://patchwork.ozlabs.org/patch/1023623/ > > > > > > if adding a new header_ops callback to parse link layer headers, > > > please have it return both protocol and link layer header length. > > Sorry, I miss the point here, can you elaborate more? If all you need is > to have some header_ops callback that returns the L2 header length, > there is one already, it's called parse. Or do you have a specific > reason why you want my callback to also return the header length? The main reason is to avoid multiple indirect function calls, both essentially doing the same: parsing the ll header. But admittedly the instances where dev->header_ops->validate are called are rare. > > This could just be an extension of existing header_ops->validate. > > If you suggest extending an existing function, parse looks more > suitable, but I decided not to touch the existing ones for two reasons: > > 1. I don't want to break the existing code that uses the parse function > and will need to be modified to pass an extra parameter. > > 2. I don't want to spend machine time on copying the destination MAC > when I only need the protocol, and vice versa. > > I'm looking forward to hearing your thoughts about it. header_ops.parse is also a good candidate. As a matter of fact, parse and validate could (eventually) probably be combined. The only direct caller to header_ops.parse appears to be dev_parse_header, so modifying its interface should be fairly straightforward. Allowing a NULL haddr could avoid the address copy cost if not needed. This does require modifying all implementations. But from a quick scan, there appear to be only 8. And only 1 for validate. So changing the implementation is quite acceptable. Another issue, though, would be what to return as protocol if a header does not encode that. Given these non-trivial changes, if you prefer to just add the dedicated new callback, that's fine. We can see independently whether deduplication makes sense. With three ll header parse functions, I think that it will be. But even if so, it is better to do so as a stand-alone noop patch than combining refactoring and new features, anyway. Long story short, sounds good. Thanks.
diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c index 8fc76e68777a..d1d89749a17a 100644 --- a/net/packet/af_packet.c +++ b/net/packet/af_packet.c @@ -1850,6 +1850,15 @@ static int packet_rcv_spkt(struct sk_buff *skb, struct net_device *dev, return 0; } +static void packet_parse_headers(struct sk_buff *skb, struct socket *sock) +{ + if (!skb->protocol && sock->type == SOCK_RAW) { + skb_reset_mac_header(skb); + skb->protocol = dev_parse_header_protocol(skb); + } + + skb_try_probe_transport_header(skb); +} /* * Output a raw packet to a device layer. This bypasses all the other @@ -1970,7 +1979,7 @@ static int packet_sendmsg_spkt(struct socket *sock, struct msghdr *msg, if (unlikely(extra_len == 4)) skb->no_fcs = 1; - skb_try_probe_transport_header(skb); + packet_parse_headers(skb, sock); dev_queue_xmit(skb); rcu_read_unlock(); @@ -2519,7 +2528,7 @@ static int tpacket_fill_skb(struct packet_sock *po, struct sk_buff *skb, len = ((to_write > len_max) ? len_max : to_write); } - skb_try_probe_transport_header(skb); + packet_parse_headers(skb, sock); return tp_len; } @@ -2924,7 +2933,7 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len) virtio_net_hdr_set_proto(skb, &vnet_hdr); } - skb_try_probe_transport_header(skb); + packet_parse_headers(skb, sock); if (unlikely(extra_len == 4)) skb->no_fcs = 1;
If a socket was created with socket(AF_PACKET, SOCK_RAW, 0), the protocol number is unavailable. Try to ask the driver to extract it from the L2 header in order for skb_try_probe_transport_header to succeed. Signed-off-by: Maxim Mikityanskiy <maximmi@mellanox.com> --- net/packet/af_packet.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-)