| Message ID | 20170207205721.23207-3-willemdebruijn.kernel@gmail.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
On Tue, 2017-02-07 at 15:57 -0500, Willem de Bruijn wrote: > From: Willem de Bruijn <willemb@google.com> > > Link layer protocols may unconditionally pull headers, as Ethernet > does in eth_type_trans. Ensure that the entire link layer header > always lies in the skb linear segment. tpacket_snd has such a check. > Extend this to packet_snd. > > Variable length link layer headers complicate the computation > somewhat. Here skb->len may be smaller than dev->hard_header_len. > > Round up the linear length to be at least as long as the smallest of > the two. > > Reported-by: Dmitry Vyukov <dvyukov@google.com> > Signed-off-by: Willem de Bruijn <willemb@google.com> > --- Acked-by: Eric Dumazet <edumazet@google.com>
On (02/07/17 15:57), Willem de Bruijn wrote: > @@ -2816,8 +2816,9 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len) > err = -ENOBUFS; > hlen = LL_RESERVED_SPACE(dev); > tlen = dev->needed_tailroom; > - skb = packet_alloc_skb(sk, hlen + tlen, hlen, len, > - __virtio16_to_cpu(vio_le(), vnet_hdr.hdr_len), > + linear = __virtio16_to_cpu(vio_le(), vnet_hdr.hdr_len); > + linear = max(linear, min_t(int, len, dev->hard_header_len)); > + skb = packet_alloc_skb(sk, hlen + tlen, hlen, len, linear, > msg->msg_flags & MSG_DONTWAIT, &err); do we need a similar check in packet_sendsmg_spkt (even if it is deprecated, would be better to get it to align with packet_snd?) Also tpacket_fill_skb should ensure that copylen is set up like the code above? --Sowmini
On Wed, Feb 8, 2017 at 7:34 AM, Sowmini Varadhan <sowmini.varadhan@oracle.com> wrote: > On (02/07/17 15:57), Willem de Bruijn wrote: >> @@ -2816,8 +2816,9 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len) >> err = -ENOBUFS; >> hlen = LL_RESERVED_SPACE(dev); >> tlen = dev->needed_tailroom; >> - skb = packet_alloc_skb(sk, hlen + tlen, hlen, len, >> - __virtio16_to_cpu(vio_le(), vnet_hdr.hdr_len), >> + linear = __virtio16_to_cpu(vio_le(), vnet_hdr.hdr_len); >> + linear = max(linear, min_t(int, len, dev->hard_header_len)); >> + skb = packet_alloc_skb(sk, hlen + tlen, hlen, len, linear, >> msg->msg_flags & MSG_DONTWAIT, &err); > > do we need a similar check in packet_sendsmg_spkt (even if it > is deprecated, would be better to get it to align with packet_snd?) That path does not need this check, because it does not support large packets and always allocates the entire packet as linear. > Also tpacket_fill_skb should ensure that copylen is set up like > the code above? Do you mean the difference that it unconditionally pulls hard_header_len, optionally with zero padding, whereas this new path can check against new min_header_len and thus allows packets shorter than hard_header_len?
On (02/08/17 08:37), Willem de Bruijn wrote: > Date: Wed, 8 Feb 2017 08:37:19 -0800 > From: Willem de Bruijn <willemdebruijn.kernel@gmail.com> > To: Sowmini Varadhan <sowmini.varadhan@oracle.com> > Cc: Network Development <netdev@vger.kernel.org>, David Miller > <davem@davemloft.net>, Eric Dumazet <eric.dumazet@gmail.com>, Dmitry > Vyukov <dvyukov@google.com>, Willem de Bruijn <willemb@google.com> > Subject: Re: [PATCH net 2/2] packet: round up linear to header len > > On Wed, Feb 8, 2017 at 7:34 AM, Sowmini Varadhan > <sowmini.varadhan@oracle.com> wrote: > > On (02/07/17 15:57), Willem de Bruijn wrote: > >> @@ -2816,8 +2816,9 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len) > >> err = -ENOBUFS; > >> hlen = LL_RESERVED_SPACE(dev); > >> tlen = dev->needed_tailroom; > >> - skb = packet_alloc_skb(sk, hlen + tlen, hlen, len, > >> - __virtio16_to_cpu(vio_le(), vnet_hdr.hdr_len), > >> + linear = __virtio16_to_cpu(vio_le(), vnet_hdr.hdr_len); > >> + linear = max(linear, min_t(int, len, dev->hard_header_len)); > >> + skb = packet_alloc_skb(sk, hlen + tlen, hlen, len, linear, > >> msg->msg_flags & MSG_DONTWAIT, &err); > > : > Do you mean the difference that it unconditionally pulls > hard_header_len, optionally with zero padding, whereas this new > path can check against new min_header_len and thus allows > packets shorter than hard_header_len? yes, maybe it doesnt matter, becaues hard_header_len >= min_header_len at all times --Sowmini
On Wed, Feb 8, 2017 at 8:58 AM, Sowmini Varadhan <sowmini.varadhan@oracle.com> wrote: > On (02/08/17 08:37), Willem de Bruijn wrote: >> Date: Wed, 8 Feb 2017 08:37:19 -0800 >> From: Willem de Bruijn <willemdebruijn.kernel@gmail.com> >> To: Sowmini Varadhan <sowmini.varadhan@oracle.com> >> Cc: Network Development <netdev@vger.kernel.org>, David Miller >> <davem@davemloft.net>, Eric Dumazet <eric.dumazet@gmail.com>, Dmitry >> Vyukov <dvyukov@google.com>, Willem de Bruijn <willemb@google.com> >> Subject: Re: [PATCH net 2/2] packet: round up linear to header len >> >> On Wed, Feb 8, 2017 at 7:34 AM, Sowmini Varadhan >> <sowmini.varadhan@oracle.com> wrote: >> > On (02/07/17 15:57), Willem de Bruijn wrote: >> >> @@ -2816,8 +2816,9 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len) >> >> err = -ENOBUFS; >> >> hlen = LL_RESERVED_SPACE(dev); >> >> tlen = dev->needed_tailroom; >> >> - skb = packet_alloc_skb(sk, hlen + tlen, hlen, len, >> >> - __virtio16_to_cpu(vio_le(), vnet_hdr.hdr_len), >> >> + linear = __virtio16_to_cpu(vio_le(), vnet_hdr.hdr_len); >> >> + linear = max(linear, min_t(int, len, dev->hard_header_len)); >> >> + skb = packet_alloc_skb(sk, hlen + tlen, hlen, len, linear, >> >> msg->msg_flags & MSG_DONTWAIT, &err); >> > > : >> Do you mean the difference that it unconditionally pulls >> hard_header_len, optionally with zero padding, whereas this new >> path can check against new min_header_len and thus allows >> packets shorter than hard_header_len? > > yes, maybe it doesnt matter, becaues hard_header_len >= min_header_len > at all times The code is not subject to this bug, so I'd rather not touch it in this fix for stable. But you raise a good point. This logic is subtle and fragile. It will be good to deduplicate across packet_snd and tpacket_snd in a follow-up to net-next.
diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c index 3d555c79a7b5..d56ee46b11fc 100644 --- a/net/packet/af_packet.c +++ b/net/packet/af_packet.c @@ -2755,7 +2755,7 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len) struct virtio_net_hdr vnet_hdr = { 0 }; int offset = 0; struct packet_sock *po = pkt_sk(sk); - int hlen, tlen; + int hlen, tlen, linear; int extra_len = 0; /* @@ -2816,8 +2816,9 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len) err = -ENOBUFS; hlen = LL_RESERVED_SPACE(dev); tlen = dev->needed_tailroom; - skb = packet_alloc_skb(sk, hlen + tlen, hlen, len, - __virtio16_to_cpu(vio_le(), vnet_hdr.hdr_len), + linear = __virtio16_to_cpu(vio_le(), vnet_hdr.hdr_len); + linear = max(linear, min_t(int, len, dev->hard_header_len)); + skb = packet_alloc_skb(sk, hlen + tlen, hlen, len, linear, msg->msg_flags & MSG_DONTWAIT, &err); if (skb == NULL) goto out_unlock;