On Thu, Aug 6, 2020 at 3:51 AM Xie He <xie.he.0...@gmail.com> wrote: > > 1. Added a skb->len check > > This driver expects upper layers to include a pseudo header of 1 byte > when passing down a skb for transmission. This driver will read this > 1-byte header. This patch added a skb->len check before reading the > header to make sure the header exists. > > 2. Changed to use needed_headroom instead of hard_header_len to request > necessary headroom to be allocated > > In net/packet/af_packet.c, the function packet_snd first reserves a > headroom of length (dev->hard_header_len + dev->needed_headroom). > Then if the socket is a SOCK_DGRAM socket, it calls dev_hard_header, > which calls dev->header_ops->create, to create the link layer header. > If the socket is a SOCK_RAW socket, it "un-reserves" a headroom of > length (dev->hard_header_len), and assumes the user to provide the > appropriate link layer header. > > So according to the logic of af_packet.c, dev->hard_header_len should > be the length of the header that would be created by > dev->header_ops->create. > > However, this driver doesn't provide dev->header_ops, so logically > dev->hard_header_len should be 0. > > So we should use dev->needed_headroom instead of dev->hard_header_len > to request necessary headroom to be allocated. > > This change fixes kernel panic when this driver is used with AF_PACKET > SOCK_RAW sockets. > > Call stack when panic: > > [ 168.399197] skbuff: skb_under_panic: text:ffffffff819d95fb len:20 > put:14 head:ffff8882704c0a00 data:ffff8882704c09fd tail:0x11 end:0xc0 > dev:veth0 > ... > [ 168.399255] Call Trace: > [ 168.399259] skb_push.cold+0x14/0x24 > [ 168.399262] eth_header+0x2b/0xc0 > [ 168.399267] lapbeth_data_transmit+0x9a/0xb0 [lapbether] > [ 168.399275] lapb_data_transmit+0x22/0x2c [lapb] > [ 168.399277] lapb_transmit_buffer+0x71/0xb0 [lapb] > [ 168.399279] lapb_kick+0xe3/0x1c0 [lapb] > [ 168.399281] lapb_data_request+0x76/0xc0 [lapb] > [ 168.399283] lapbeth_xmit+0x56/0x90 [lapbether] > [ 168.399286] dev_hard_start_xmit+0x91/0x1f0 > [ 168.399289] ? irq_init_percpu_irqstack+0xc0/0x100 > [ 168.399291] __dev_queue_xmit+0x721/0x8e0 > [ 168.399295] ? packet_parse_headers.isra.0+0xd2/0x110 > [ 168.399297] dev_queue_xmit+0x10/0x20 > [ 168.399298] packet_sendmsg+0xbf0/0x19b0 > ...... > > Cc: Willem de Bruijn <willemdebruijn.ker...@gmail.com> > Cc: Martin Schiller <m...@dev.tdt.de> > Cc: Brian Norris <briannor...@chromium.org> > Signed-off-by: Xie He <xie.he.0...@gmail.com>
Acked-by: Willem de Bruijn <will...@google.com> The in-band signal byte is required, but stripped by lapbeth_xmit. Subsequent code will prefix additional headers, including an Ethernet link layer. The extra space needs to be reserved, but not pulled in packet_snd with skb_reserve, so has to use needed_headroom instead of hard_header_len.
