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

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

[Xie He]

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.

Signed-off-by: Xie He <xie.he.0141@gmail.com>
---

Patch v2 has no difference from v1.
I re-submitted it because I want to find new reviewers,
and I want to free new reviewers from the burden of reading the
lengthy discussion and explanations in the v1 email threads.

Summary of v1 discussion:
Cong Wang referred me to Brian Norris, who did a similar change before.
Brian Norris agreed with me on "hard_header_len vs needed_headroom",
but was unfamiliar with X.25 drivers.

---
 drivers/net/wan/lapbether.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wan/lapbether.c b/drivers/net/wan/lapbether.c
index b2868433718f..34cf6db89912 100644
--- a/drivers/net/wan/lapbether.c
+++ b/drivers/net/wan/lapbether.c
@@ -305,6 +305,7 @@ static void lapbeth_setup(struct net_device *dev)
 	dev->netdev_ops	     = &lapbeth_netdev_ops;
 	dev->needs_free_netdev = true;
 	dev->type            = ARPHRD_X25;
+	dev->hard_header_len = 0;
 	dev->mtu             = 1000;
 	dev->addr_len        = 0;
 }
@@ -331,7 +332,8 @@ static int lapbeth_new_device(struct net_device *dev)
 	 * then this driver prepends a length field of 2 bytes,
 	 * then the underlying Ethernet device prepends its own header.
 	 */
-	ndev->hard_header_len = -1 + 3 + 2 + dev->hard_header_len;
+	ndev->needed_headroom = -1 + 3 + 2 + dev->hard_header_len
+					   + dev->needed_headroom;
 
 	lapbeth = netdev_priv(ndev);
 	lapbeth->axdev = ndev;
-- 
2.25.1

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

[Xie He]

Hi Martin,

I'm currently working on a plan to make all X.25 drivers (lapbether.c,
x25_asy.c, hdlc_x25.c) to set dev->hard_header_len /
dev->needed_headroom correctly. So that upper layers no longer need to
guess how much headroom a X.25 device needs with a constant value (as
they currently do).

After studying af_packet.c, I found that X.25 drivers needed to set
needed_headroom to reserve the headroom instead of using
hard_header_len. Because hard_header_len should be the length of the
header that would be created by dev_hard_header, and in this case it
should be 0, according to the logic of af_packet.c.

So my first step is to fix the settings in lapbether.c. Could you
review this patch and extend your support via a "Reviewed-by" tag? If
this can be fixed, I'll go on and fix other X.25 drivers. Thanks!

It's very hard to find reviewers for X.25 code because it is
relatively unmaintained by people. I hope I can do some of the
maintenance work. I greatly appreciate your support!

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

[Xie He]

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.

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

[Xie He]

Brian Norris has approved this patch with "Reviewed-by" in the v1
email thread. I really appreciate his review. It's very hard for me to
find reviewers for X.25 code so I'm grateful for anyone who could
help. Thanks to everyone.

Reviewed-by: Brian Norris <briannorris@chromium.org>

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

[Willem de Bruijn]

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/

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

[Xie He]

Thank you for your thorough review comment!

On Fri, Jul 31, 2020 at 7:13 AM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> 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.

> 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.

> 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.

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.

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

[Willem de Bruijn]

On Fri, Jul 31, 2020 at 4:41 PM Xie He <xie.he.0141@gmail.com> wrote:
>
> Thank you for your thorough review comment!
>
> On Fri, Jul 31, 2020 at 7:13 AM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
> >
> > 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.

> > 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.

> > 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.

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

[Xie He]

On Fri, Jul 31, 2020 at 7:33 PM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> 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?

Yes, this 1-byte header is just a "fake" header that is only for
communication between the network layer and the link layer. It never
ends up on wire.

I think we can think of it as the Ethernet header for Wifi drivers.
Although Wifi doesn't actually use the Ethernet header, Wifi drivers
use a "fake" Ethernet header to communicate with code outside of the
driver. From outside, it appears that Wifi drivers use the Ethernet
header.

> > 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.

Actually I tried this solution today. It was easier to implement than
I originally thought. I implemented header_ops to make dev_hard_header
generate the 1-byte header. And when receiving, (according to the
requirement of af_packet.c) I pulled this 1-byte header before
submitting the packet to upper layers. Everything worked fine, except
one issue:

When receiving, af_packet.c doesn't handle 0-sized packets well. It
will drop them. This causes an AF_PACKET/DGRAM socket to receive no
indication when it is connected or disconnected. Do you think this is
a problem? Actually I'm also afraid that future changes in af_packet.c
will make 0-sized packets not able to pass when sending as well.

> 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.

Yes, I think the first solution may be better, because we need to have
a way to drop 0-sized DGRAM packets (as long as we need to include the
1-byte header when sending DGRAM packets) and I'm not aware
af_packet.c can do this.

Yes, I think maybe the best way is to get rid of the 1-byte header
completely and use other ways to ask the driver to connect or
disconnect, or let it connect and disconnect automatically.

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

Thanks for pointing this out!

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

[Willem de Bruijn]

On Sat, Aug 1, 2020 at 8:46 AM Xie He <xie.he.0141@gmail.com> wrote:
>
> On Fri, Jul 31, 2020 at 7:33 PM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
> >
> > 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?
>
> Yes, this 1-byte header is just a "fake" header that is only for
> communication between the network layer and the link layer. It never
> ends up on wire.
>
> I think we can think of it as the Ethernet header for Wifi drivers.
> Although Wifi doesn't actually use the Ethernet header, Wifi drivers
> use a "fake" Ethernet header to communicate with code outside of the
> driver. From outside, it appears that Wifi drivers use the Ethernet
> header.
>
> > > 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.
>
> Actually I tried this solution today. It was easier to implement than
> I originally thought. I implemented header_ops to make dev_hard_header
> generate the 1-byte header. And when receiving, (according to the
> requirement of af_packet.c) I pulled this 1-byte header before
> submitting the packet to upper layers. Everything worked fine, except
> one issue:
>
> When receiving, af_packet.c doesn't handle 0-sized packets well. It
> will drop them. This causes an AF_PACKET/DGRAM socket to receive no
> indication when it is connected or disconnected. Do you think this is
> a problem?

The kernel interface cannot be changed. If packet sockets used to pass
the first byte up to userspace, they have to continue to do so.

So I think you can limit the header_ops to only dev_hard_header.

> Actually I'm also afraid that future changes in af_packet.c
> will make 0-sized packets not able to pass when sending as well.
>
> > 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.
>
> Yes, I think the first solution may be better, because we need to have
> a way to drop 0-sized DGRAM packets (as long as we need to include the
> 1-byte header when sending DGRAM packets) and I'm not aware
> af_packet.c can do this.
>
> Yes, I think maybe the best way is to get rid of the 1-byte header
> completely and use other ways to ask the driver to connect or
> disconnect, or let it connect and disconnect automatically.

Fixes should be small and targeted. Any larger refactoring is
best addressed in a separate net-next patch.



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

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

[Xie He]

On Sat, Aug 1, 2020 at 6:31 AM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> The kernel interface cannot be changed. If packet sockets used to pass
> the first byte up to userspace, they have to continue to do so.
>
> So I think you can limit the header_ops to only dev_hard_header.

Actually if we want to keep the kernel interface unchanged, we
shouldn't implement header_ops for dev_hard_header either, because
this changes the way the user space program sends DGRAM packets, too.
Before the change the userspace program needs to add the 1-byte header
before sending, and after the change the userspace program will let
the kernel add the header via dev_hard_header.

> Fixes should be small and targeted. Any larger refactoring is
> best addressed in a separate net-next patch.

I guess the best way for this fix patch would be just add a 0-byte
packet check before the driver reads skb->data[0].

Thanks! I'll add the check and re-send the patch.

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

[Martin Schiller]

On 2020-07-30 10:02, Xie He wrote:
> Hi Martin,
> 
> I'm currently working on a plan to make all X.25 drivers (lapbether.c,
> x25_asy.c, hdlc_x25.c) to set dev->hard_header_len /
> dev->needed_headroom correctly. So that upper layers no longer need to
> guess how much headroom a X.25 device needs with a constant value (as
> they currently do).
> 
> After studying af_packet.c, I found that X.25 drivers needed to set
> needed_headroom to reserve the headroom instead of using
> hard_header_len. Because hard_header_len should be the length of the
> header that would be created by dev_hard_header, and in this case it
> should be 0, according to the logic of af_packet.c.
> 
> So my first step is to fix the settings in lapbether.c. Could you
> review this patch and extend your support via a "Reviewed-by" tag? If
> this can be fixed, I'll go on and fix other X.25 drivers. Thanks!
> 
> It's very hard to find reviewers for X.25 code because it is
> relatively unmaintained by people. I hope I can do some of the
> maintenance work. I greatly appreciate your support!

I don't like the idea to get rid of the 1-byte header.
This header is also used in userspace, for example when using a tun/tap
interface for an XoT (X.25 over TCP) application. A change would
therefore have very far-reaching consequences.

BTW: The linux x25 mailing list does not seem to work anymore. I've been
on it for some time now, but haven't received a single email from it.
I've tried to contact owner-linux-x25@vger.kernel.org, but only got an
"undeliverable" email back.

It would be great if you could add me to CC list of all versions of your
patches, so I don't need to "google" for any further related mails.

So, what's the latest version of the patch now, which you want me to
review?

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

[Willem de Bruijn]

> I don't like the idea to get rid of the 1-byte header.
> This header is also used in userspace, for example when using a tun/tap
> interface for an XoT (X.25 over TCP) application. A change would
> therefore have very far-reaching consequences.

That's no longer the plan of record.

> BTW: The linux x25 mailing list does not seem to work anymore. I've been
> on it for some time now, but haven't received a single email from it.
> I've tried to contact owner-linux-x25@vger.kernel.org, but only got an
> "undeliverable" email back.

That is odd. It is a vger hosted list.

I'm not subscribed, but indeed the spinics archive ends in 2009 and
the other archive link resolves to something that is definitely not
X.25 related.

http://vger.kernel.org/vger-lists.html#linux-x25

> It would be great if you could add me to CC list of all versions of your
> patches, so I don't need to "google" for any further related mails.
>
> So, what's the latest version of the patch now, which you want me to
> review?

http://patchwork.ozlabs.org/project/netdev/patch/20200802195046.402539-1-xie.he.0141@gmail.com/

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

[Xie He]

On Mon, Aug 3, 2020 at 11:53 PM Martin Schiller <ms@dev.tdt.de> wrote:
>
> I don't like the idea to get rid of the 1-byte header.
> This header is also used in userspace, for example when using a tun/tap
> interface for an XoT (X.25 over TCP) application. A change would
> therefore have very far-reaching consequences.

Thank you for your comment! This is very important information to me.
Now I think it may be the best to keep the 1-byte header so that the
kernel interface can be kept unchanged.

> BTW: The linux x25 mailing list does not seem to work anymore. I've been
> on it for some time now, but haven't received a single email from it.
> I've tried to contact owner-linux-x25@vger.kernel.org, but only got an
> "undeliverable" email back.

I was suspecting that it was not working, too. I CC'd all my patches
to the mail list but got no response from it. It appears that you were
not able to receive my emails through it, too.

> It would be great if you could add me to CC list of all versions of your
> patches, so I don't need to "google" for any further related mails.

OK. I'll surely do that! Thank you for taking time to review my patches!

> So, what's the latest version of the patch now, which you want me to
> review?

It is at:
http://patchwork.ozlabs.org/project/netdev/patch/20200802195046.402539-1-xie.he.0141@gmail.com/

Thank you so much for your review!

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

[Xie He]

On Tue, Aug 4, 2020 at 12:06 AM Willem de Bruijn <willemb@google.com> wrote:
>
> > BTW: The linux x25 mailing list does not seem to work anymore. I've been
> > on it for some time now, but haven't received a single email from it.
> > I've tried to contact owner-linux-x25@vger.kernel.org, but only got an
> > "undeliverable" email back.
>
> That is odd. It is a vger hosted list.
>
> I'm not subscribed, but indeed the spinics archive ends in 2009 and
> the other archive link resolves to something that is definitely not
> X.25 related.
>
> http://vger.kernel.org/vger-lists.html#linux-x25

Maybe we could contact <majordomo@vger.kernel.org>. It seems to be the
manager of VGER mail lists.

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

[Xie He]

On Tue, Aug 4, 2020 at 3:07 AM Xie He <xie.he.0141@gmail.com> wrote:
>
> Maybe we could contact <majordomo@vger.kernel.org>. It seems to be the
> manager of VGER mail lists.

Oh. No. Majordomo seems to be a robot.