Re: [PATCH v2] drivers/net/wan/lapbether: Use needed_headroom instead of hard_header_len

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

On Thu, Jul 30, 2020 at 9:36 PM Xie He <xie.he.0141@gmail.com> wrote:
>
> I'm really sorry to have re-sent the patch when the patch is still in
> review. I don't intend to be disrespectful to anyone. And I apologize
> for any disrespectfulness this might appear. Sorry.
>
> I'm also sorry for not having sent the patch with the proper subject
> prefixed with "net" or "net-next". If anyone requests I can re-send
> this patch with the proper subject "PATCH net".
>
> This patch actually fixes a kernel panic when this driver is used with
> a AF_PACKET/RAW socket. This driver runs on top of Ethernet
> interfaces. So I created a pair of virtual Ethernet (veth) interfaces,
> loaded this driver to create a pair of X.25 interfaces on top of them,
> and wrote C programs to use AF_PACKET sockets to send/receive data
> through them.
>
> At first I used AF_PACKET/DGRAM sockets. I prepared packet data
> according to the requirements of X.25 drivers. I first sent an
> one-byte packet ("\x01") to instruct the driver to connect, then I
> sent data prefixed with an one-byte pseudo header ("\x00") to instruct
> the driver to send the data, and then I sent another one-byte packet
> ("\x02") to instruct the driver to disconnect.
>
> This works fine with AF_PACKET/DGRAM sockets. However, when I change
> it to AF_PACKET/RAW sockets, kernel panic occurs. The stack trace is
> as follows. We can see the kernel panicked because of insufficient
> header space when pushing the Ethernet header.
>
> [  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
> ......
>
> After applying this patch, the kernel panic no longer appears, and
> AF_PACKET/RAW sockets would then behave the same as AF_PACKET/DGRAM
> sockets.

Thanks for fixing a kernel panic. The existing line was added recently
in commit 9dc829a135fb ("drivers/net/wan/lapbether: Fixed the value of
hard_header_len"). I assume a kernel with that commit reverted also
panics? It does looks like it would.

If this driver submits a modified packet to an underlying eth device,
it is akin to tunnel drivers. The hard_header_len vs needed_headroom
discussion also came up there recently [1]. That discussion points to
commit c95b819ad75b ("gre: Use needed_headroom"). So the general
approach in this patch is fine. Do note the point about mtu
calculations -- but this device just hardcodes a 1000 byte dev->mtu
irrespective of underlying ethernet device mtu, so I guess it has
bigger issues on that point.

But, packet sockets with SOCK_RAW have to pass a fully formed packet
with all the headers the ndo_start_xmit expects, i.e., it should be
safe for the device to just pull that many bytes. X25 requires the
peculiar one byte pseudo header you mention: lapbeth_xmit
unconditionally reads skb->data[0] and then calls skb_pull(skb, 1).
This could be considered the device hard header len.

[1] https://lore.kernel.org/netdev/86c71cc0-462c-2365-00ea-7f9e79c204b7@6wind.com/