On Fri, Jul 31, 2020 at 4:41 PM Xie He [off-list ref] wrote:

Thank you for your thorough review comment!

On Fri, Jul 31, 2020 at 7:13 AM Willem de Bruijn
[off-list ref] wrote:

quoted

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.

Yes, that commit also fixed kernel panic. But that patch only fixed
kernel panic when using AF_PACKET/DGRAM sockets. It didn't fix kernel
panic when using AF_PACKET/RAW sockets. This patch attempts to fix
kernel panic when using AF_PACKET/RAW sockets, too.

Ah, okay. That's good to know.

While this protocol is old and seemingly unmaintained, it probably is
still in use. But the packet interface is not the common datapath. We
have to be careful not to introduce regressions to that.

quoted

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.

Yes, I didn't consider the issue of mtu calculation. Maybe we need to
calculate the mtu of this device based on the underlying Ethernet
device, too.

We may also need to handle the situation where the mtu of the
underlying Ethernet device changes.

I'm not sure if the mtu of the device can be changed by the user
without explicit support from the driver. If it can, we may also need
to set max_mtu and min_mtu properly to prevent the user from setting
it to invalid values.

I suggest to ignore mtu. It is out of scope of this patch, which does
address an unrelated real kernel panic.

quoted

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.

Yes, I agree that we can use hard_header_len (and min_header_len) to
prevent packets shorter than 1 byte from passing.

But because af_packet.c reserves a header space of needed_headroom for
RAW sockets, but hard_header_len + needed_headroom for DGRAM sockets,
it appears to me that af_packet.c expects hard_header_len to be the
header length created by dev_hard_header. We can, however, set
hard_header_len to 1 and let dev_hard_header generate a 0-sized
header, but this makes af_packet.c to reserve an extra unused 1-byte
header space for DGRAM sockets, and DGRAM sockets will not be
protected by the 1-byte minimum length check like RAW sockets.

Good point.

The best solution might be to implement header_ops for X.25 drivers
and let dev_hard_header create this 1-byte header, so that
hard_header_len can equal to the header length created by
dev_hard_header. This might be the best way to fit the logic of
af_packet.c. But this requires changing the interface of X.25 drivers
so it might be a big change.

Agreed.

I quickly scanned the main x.25 datapath code. Specifically
x25_establish_link, x25_terminate_link and x25_send_frame. These all
write this 1 byte header. It appears to be an in-band communication
means between the network and data link layer, never actually ending
up on the wire?

Either lapbeth_xmit has to have a guard against 0 byte packets before
reading skb->data[0], or packet sockets should not be able to generate
those (is this actually possible today through PF_PACKET? not sure)

If SOCK_DGRAM has to always select one of the three values (0x00:
data, 0x01: establish, 0x02: terminate) the first seems most sensible.
Though if there is no way to establish a connection with
PF_PACKET/SOCK_DGRAM, that whole interface may still be academic.
Maybe eventually either 0x00 or 0x01 could be selected based on
lapb->state.. That however is out of scope of this fix.

Normally a fix should aim to have a Fixes: tag, but all this code
precedes git history, so that is not feasible here.