| Message ID | 20200730073702.16887-1-xie.he.0141@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | David Miller |
| Headers | show |
| Series | [v2] drivers/net/wan/lapbether: Use needed_headroom instead of hard_header_len | expand |
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'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.
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>
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/
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.
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.
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!
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!
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.
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?
> 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/
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!
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.
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.
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;
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(-)