[PATCH net] net/core/dev.c: Ensure pfmemalloc skbs are correctly handled when receiving
When an skb is allocated by "__netdev_alloc_skb" in "net/core/skbuff.c",
if "sk_memalloc_socks()" is true, and if there's not sufficient memory,
the skb would be allocated using emergency memory reserves. This kind of
skbs are called pfmemalloc skbs.
pfmemalloc skbs must be specially handled in "net/core/dev.c" when
receiving. They must NOT be delivered to the target protocol if
"skb_pfmemalloc_protocol(skb)" is false.
However, if, after a pfmemalloc skb is allocated and before it reaches
the code in "__netif_receive_skb", "sk_memalloc_socks()" becomes false,
then the skb will be handled by "__netif_receive_skb" as a normal skb.
This causes the skb to be delivered to the target protocol even if
"skb_pfmemalloc_protocol(skb)" is false.
This patch fixes this problem by ensuring all pfmemalloc skbs are handled
by "__netif_receive_skb" as pfmemalloc skbs.
"__netif_receive_skb_list" has the same problem as "__netif_receive_skb".
This patch also fixes it.
Fixes: b4b9e3558508 ("netvm: set PF_MEMALLOC as appropriate during SKB
processing")
Cc: Mel Gorman
Cc: David S. Miller
Cc: Neil Brown
Cc: Peter Zijlstra
Cc: Jiri Slaby
Cc: Mike Christie
Cc: Eric B Munson
Cc: Eric Dumazet
Cc: Sebastian Andrzej Siewior
Cc: Christoph Lameter
Cc: Andrew Morton
Signed-off-by: Xie He
---
net/core/dev.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/core/dev.c b/net/core/dev.c
index 1f79b9aa9a3f..3e6b7879daef 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5479,7 +5479,7 @@ static int __netif_receive_skb(struct sk_buff *skb)
{
int ret;
- if (sk_memalloc_socks() && skb_pfmemalloc(skb)) {
+ if (skb_pfmemalloc(skb)) {
unsigned int noreclaim_flag;
/*
@@ -5507,7 +5507,7 @@ static void __netif_receive_skb_list(struct list_head
*head)
bool pfmemalloc = false; /* Is current sublist PF_MEMALLOC? */
list_for_each_entry_safe(skb, next, head, list) {
- if ((sk_memalloc_socks() && skb_pfmemalloc(skb)) != pfmemalloc)
{
+ if (skb_pfmemalloc(skb) != pfmemalloc) {
struct list_head sublist;
/* Handle the previous sublist */
--
2.27.0
Re: Problem in pfmemalloc skb handling in net/core/dev.c
On Fri, Apr 9, 2021 at 12:12 PM Xie He wrote: > > This is exactly what I'm talking about. "skb_pfmemalloc_protocol" > cannot guarantee pfmemalloc skbs are not delivered to unrelated > protocols, because "__netif_receive_skb" will sometimes treat > pfmemalloc skbs as normal skbs. > I'm not sure if you understand what I'm saying. Please look at the > code of "__netif_receive_skb" and see what will happen when > "sk_memalloc_socks()" is false and "skb_pfmemalloc(skb)" is true. Do you see the problem now? Just think what happens when "skb_pfmemalloc(skb)" is true and "sk_memalloc_socks()" has just changed to "false", and whether in this case "skb_pfmemalloc_protocol" still takes any effect.
Re: Problem in pfmemalloc skb handling in net/core/dev.c
On Fri, Apr 9, 2021 at 4:50 AM Eric Dumazet wrote: > > On 4/9/21 12:14 PM, Xie He wrote: > > Then simply copy the needed logic. No, there's no such thing as "sockets" in some of the protocols. There is simply no way to copy "the needed logic". > > Also, I think this is a problem in net/core/dev.c, there are a lot of > > old protocols that are not aware of pfmemalloc skbs. I don't think > > it's a good idea to fix them one by one. > > > > I think you are mistaken. > > There is no problem in net/core/dev.c really, it uses > skb_pfmemalloc_protocol() This is exactly what I'm talking about. "skb_pfmemalloc_protocol" cannot guarantee pfmemalloc skbs are not delivered to unrelated protocols, because "__netif_receive_skb" will sometimes treat pfmemalloc skbs as normal skbs. > pfmemalloc is best effort really. > > If a layer store packets in many long living queues, it has to drop > pfmemalloc packets, > unless these packets are used for swapping. Yes, the code of "net/core/dev.c" has exactly this problem. It doesn't drop pfmemalloc skbs in some situations, and instead deliver them to unrelated protocols, which clearly have nothing to do with swapping. I'm not sure if you understand what I'm saying. Please look at the code of "__netif_receive_skb" and see what will happen when "sk_memalloc_socks()" is false and "skb_pfmemalloc(skb)" is true.
Re: Problem in pfmemalloc skb handling in net/core/dev.c
On Fri, Apr 9, 2021 at 3:04 AM Eric Dumazet wrote: > > Note that pfmemalloc skbs are normally dropped in sk_filter_trim_cap() > > Simply make sure your protocol use it. It seems "sk_filter_trim_cap" needs an "struct sock" argument. Some of my protocols act like a middle layer to another protocol and don't have any "struct sock". Also, I think this is a problem in net/core/dev.c, there are a lot of old protocols that are not aware of pfmemalloc skbs. I don't think it's a good idea to fix them one by one.
Re: Problem in pfmemalloc skb handling in net/core/dev.c
On Fri, Apr 9, 2021 at 2:58 AM Mel Gorman wrote: > > On Fri, Apr 09, 2021 at 02:14:12AM -0700, Xie He wrote: > > > > Do you mean that at the time "sk_memalloc_socks()" changes from "true" > > to "false", there would be no in-flight skbs currently being received, > > and all network communications have been paused? > > Not all network communication, but communication with swap devices > should have stopped once sk_memalloc_socks is false. But all incoming network traffic can be allocated as pfmemalloc skbs, regardless whether or not it is related to swap devices. My protocols don't need and cannot handle pfmemalloc skbs, therefore I want to make sure my protocols never receive pfmemalloc skbs. The current code doesn't seem to guarantee this.
Re: Problem in pfmemalloc skb handling in net/core/dev.c
On Fri, Apr 9, 2021 at 1:44 AM Mel Gorman wrote: > > That would imply that the tap was communicating with a swap device to > allocate a pfmemalloc skb which shouldn't happen. Furthermore, it would > require the swap device to be deactivated while pfmemalloc skbs still > existed. Have you encountered this problem? I'm not a user of swap devices or pfmemalloc skbs. I just want to make sure the protocols that I'm developing (not IP or IPv6) won't get pfmemalloc skbs when receiving, because those protocols cannot handle them. According to the code, it seems always possible to get a pfmemalloc skb when a network driver calls "__netdev_alloc_skb". The skb will then be queued in per-CPU backlog queues when the driver calls "netif_rx". There seems to be nothing preventing "sk_memalloc_socks()" from becoming "false" after the skb is allocated and before it is handled by "__netif_receive_skb". Do you mean that at the time "sk_memalloc_socks()" changes from "true" to "false", there would be no in-flight skbs currently being received, and all network communications have been paused?
Re: Problem in pfmemalloc skb handling in net/core/dev.c
On Fri, Apr 9, 2021 at 12:30 AM Mel Gorman wrote: > > Under what circumstances do you expect sk_memalloc_socks() to be false > and skb_pfmemalloc() to be true that would cause a problem? For example, if at the time the skb is allocated, "sk_memalloc_socks()" was true, then the skb might be allocated as a pfmemalloc skb. However, if after this skb is allocated and before this skb reaches "__netif_receive_skb", "sk_memalloc_socks()" has changed from "true" to "false", then "__netif_receive_skb" will see "sk_memalloc_socks()" being false and "skb_pfmemalloc(skb)" being true. This is a problem because this would cause a pfmemalloc skb to be delivered to "taps" and protocols that don't support pfmemalloc skbs.
Problem in pfmemalloc skb handling in net/core/dev.c
Hi Mel Gorman,
I may have found a problem in pfmemalloc skb handling in
net/core/dev.c. I see there are "if" conditions checking for
"sk_memalloc_socks() && skb_pfmemalloc(skb)", and when the condition
is true, the skb is handled specially as a pfmemalloc skb, otherwise
it is handled as a normal skb.
However, if "sk_memalloc_socks()" is false and "skb_pfmemalloc(skb)"
is true, the skb is still handled as a normal skb. Is this correct?
This might happen if "sk_memalloc_socks()" was originally true and has
just turned into false before the check. Can this happen?
I found the original commit that added the "if" conditions:
commit b4b9e3558508 ("netvm: set PF_MEMALLOC as appropriate during SKB
processing")
The commit message clearly indicates pfmemalloc skbs shouldn't be
delivered to taps (or protocols that don't support pfmemalloc skbs).
However, if they are incorrectly handled as normal skbs, they could be
delivered to those places.
I'm not sure if my understanding is correct. Could you please help? Thank you!
Re: [PATCH net-next v5] net: x25: Queue received packets in the drivers instead of per-CPU queues
On Tue, Apr 6, 2021 at 4:14 PM David Miller wrote: > > This no longe applies to net-next, please respin. Oh. I see this has already been applied to net-next. Thank you! There's no need to apply again. Thanks!
Re: [PATCH net-next v5] net: x25: Queue received packets in the drivers instead of per-CPU queues
On Mon, Apr 5, 2021 at 11:17 PM Martin Schiller wrote: > > Acked-by: Martin Schiller > > Just for the record: I'm certainly not always the fastest, > but I don't work holidays or weekends. So you also need to have some > patience. Oh. Thank you! Sorry, I didn't know there was a holiday in Germany. Happy holiday! :)
Re: [PATCH net-next v5] net: x25: Queue received packets in the drivers instead of per-CPU queues
Hi Martin, Could you ack? Thanks!
[PATCH net-next v5] net: x25: Queue received packets in the drivers instead of per-CPU queues
X.25 Layer 3 (the Packet Layer) expects layer 2 to provide a reliable datalink service such that no packets are reordered or dropped. And X.25 Layer 2 (the LAPB layer) is indeed designed to provide such service. However, this reliability is not preserved when a driver calls "netif_rx" to deliver the received packets to layer 3, because "netif_rx" will put the packets into per-CPU queues before they are delivered to layer 3. If there are multiple CPUs, the order of the packets may not be preserved. The per-CPU queues may also drop packets if there are too many. Therefore, we should not call "netif_rx" to let it queue the packets. Instead, we should use our own queue that won't reorder or drop packets. This patch changes all X.25 drivers to use their own queues instead of calling "netif_rx". The patch also documents this requirement in the "x25-iface" documentation. Cc: Martin Schiller Signed-off-by: Xie He --- Change from v4 to v5: Break a line that exceeds 80 characters. Change from v1 to v4: Add the "__GFP_NOMEMALLOC" flag to "dev_alloc_skb" calls, to prevent pfmemalloc skbs from occurring. (Changes in v2 and v3 are no longer present in v4.) Change from RFC to v1: Rebase onto the latest net-next. --- Documentation/networking/x25-iface.rst | 65 -- drivers/net/wan/hdlc_x25.c | 30 ++-- drivers/net/wan/lapbether.c| 49 +-- 3 files changed, 80 insertions(+), 64 deletions(-) diff --git a/Documentation/networking/x25-iface.rst b/Documentation/networking/x25-iface.rst index df401891dce6..f34e9ec64937 100644 --- a/Documentation/networking/x25-iface.rst +++ b/Documentation/networking/x25-iface.rst @@ -70,60 +70,13 @@ First Byte = 0x03 (X25_IFACE_PARAMS) LAPB parameters. To be defined. +Requirements for the device driver +-- -Possible Problems -= - -(Henner Eisen, 2000-10-28) - -The X.25 packet layer protocol depends on a reliable datalink service. -The LAPB protocol provides such reliable service. But this reliability -is not preserved by the Linux network device driver interface: - -- With Linux 2.4.x (and above) SMP kernels, packet ordering is not - preserved. Even if a device driver calls netif_rx(skb1) and later - netif_rx(skb2), skb2 might be delivered to the network layer - earlier that skb1. -- Data passed upstream by means of netif_rx() might be dropped by the - kernel if the backlog queue is congested. - -The X.25 packet layer protocol will detect this and reset the virtual -call in question. But many upper layer protocols are not designed to -handle such N-Reset events gracefully. And frequent N-Reset events -will always degrade performance. - -Thus, driver authors should make netif_rx() as reliable as possible: - -SMP re-ordering will not occur if the driver's interrupt handler is -always executed on the same CPU. Thus, - -- Driver authors should use irq affinity for the interrupt handler. - -The probability of packet loss due to backlog congestion can be -reduced by the following measures or a combination thereof: - -(1) Drivers for kernel versions 2.4.x and above should always check the -return value of netif_rx(). If it returns NET_RX_DROP, the -driver's LAPB protocol must not confirm reception of the frame -to the peer. -This will reliably suppress packet loss. The LAPB protocol will -automatically cause the peer to re-transmit the dropped packet -later. -The lapb module interface was modified to support this. Its -data_indication() method should now transparently pass the -netif_rx() return value to the (lapb module) caller. -(2) Drivers for kernel versions 2.2.x should always check the global -variable netdev_dropping when a new frame is received. The driver -should only call netif_rx() if netdev_dropping is zero. Otherwise -the driver should not confirm delivery of the frame and drop it. -Alternatively, the driver can queue the frame internally and call -netif_rx() later when netif_dropping is 0 again. In that case, delivery -confirmation should also be deferred such that the internal queue -cannot grow to much. -This will not reliably avoid packet loss, but the probability -of packet loss in netif_rx() path will be significantly reduced. -(3) Additionally, driver authors might consider to support -CONFIG_NET_HW_FLOWCONTROL. This allows the driver to be woken up -when a previously congested backlog queue becomes empty again. -The driver could uses this for flow-controlling the peer by means -of the LAPB protocol's flow-control service. +Packets should not be reordered or dropped when delivering between the +Packet Layer and the device driver. + +To avoid packets from being reordered or dropped when delivering from +the device driver to the Packet Layer, the device driver should not +cal
Re: [PATCH net-next v4] net: x25: Queue received packets in the drivers instead of per-CPU queues
On Thu, Apr 1, 2021 at 3:06 AM Martin Schiller wrote: > > Hi! > > Sorry for my late answer. > Can you please also fix this line length warning? > https://patchwork.hopto.org/static/nipa/442445/12117801/checkpatch/stdout OK! Sure! I'll fix the line length warning and resubmit. Thanks!
Re: [PATCH net-next v4] net: x25: Queue received packets in the drivers instead of per-CPU queues
Hi Martin, Could you ack this patch again? The only change from the RFC version (that you previously acked) is the addition of the "__GFP_NOMEMALLOC" flag in "dev_alloc_skb". This is because I want to prevent pfmemalloc skbs (which can't be handled by netif_receive_skb_core) from occurring. Thanks!
[PATCH net-next v4] net: x25: Queue received packets in the drivers instead of per-CPU queues
X.25 Layer 3 (the Packet Layer) expects layer 2 to provide a reliable datalink service such that no packets are reordered or dropped. And X.25 Layer 2 (the LAPB layer) is indeed designed to provide such service. However, this reliability is not preserved when a driver calls "netif_rx" to deliver the received packets to layer 3, because "netif_rx" will put the packets into per-CPU queues before they are delivered to layer 3. If there are multiple CPUs, the order of the packets may not be preserved. The per-CPU queues may also drop packets if there are too many. Therefore, we should not call "netif_rx" to let it queue the packets. Instead, we should use our own queue that won't reorder or drop packets. This patch changes all X.25 drivers to use their own queues instead of calling "netif_rx". The patch also documents this requirement in the "x25-iface" documentation. Cc: Martin Schiller Signed-off-by: Xie He --- Change from v3: Remove all pfmemalloc checks. Instead, prevent these skbs from occurring. Change from v2: Remove printing of pfmemalloc check errors if LAPB module can handle them. Change from v1: Add pfmemalloc skb checks and drop all pfmemalloc skbs. Change from RFC (March 5, 2021): Rebased onto the latest net-next. Martin Schiller has acked the RFC version. --- Documentation/networking/x25-iface.rst | 65 -- drivers/net/wan/hdlc_x25.c | 30 ++-- drivers/net/wan/lapbether.c| 48 +-- 3 files changed, 79 insertions(+), 64 deletions(-) diff --git a/Documentation/networking/x25-iface.rst b/Documentation/networking/x25-iface.rst index df401891dce6..f34e9ec64937 100644 --- a/Documentation/networking/x25-iface.rst +++ b/Documentation/networking/x25-iface.rst @@ -70,60 +70,13 @@ First Byte = 0x03 (X25_IFACE_PARAMS) LAPB parameters. To be defined. +Requirements for the device driver +-- -Possible Problems -= - -(Henner Eisen, 2000-10-28) - -The X.25 packet layer protocol depends on a reliable datalink service. -The LAPB protocol provides such reliable service. But this reliability -is not preserved by the Linux network device driver interface: - -- With Linux 2.4.x (and above) SMP kernels, packet ordering is not - preserved. Even if a device driver calls netif_rx(skb1) and later - netif_rx(skb2), skb2 might be delivered to the network layer - earlier that skb1. -- Data passed upstream by means of netif_rx() might be dropped by the - kernel if the backlog queue is congested. - -The X.25 packet layer protocol will detect this and reset the virtual -call in question. But many upper layer protocols are not designed to -handle such N-Reset events gracefully. And frequent N-Reset events -will always degrade performance. - -Thus, driver authors should make netif_rx() as reliable as possible: - -SMP re-ordering will not occur if the driver's interrupt handler is -always executed on the same CPU. Thus, - -- Driver authors should use irq affinity for the interrupt handler. - -The probability of packet loss due to backlog congestion can be -reduced by the following measures or a combination thereof: - -(1) Drivers for kernel versions 2.4.x and above should always check the -return value of netif_rx(). If it returns NET_RX_DROP, the -driver's LAPB protocol must not confirm reception of the frame -to the peer. -This will reliably suppress packet loss. The LAPB protocol will -automatically cause the peer to re-transmit the dropped packet -later. -The lapb module interface was modified to support this. Its -data_indication() method should now transparently pass the -netif_rx() return value to the (lapb module) caller. -(2) Drivers for kernel versions 2.2.x should always check the global -variable netdev_dropping when a new frame is received. The driver -should only call netif_rx() if netdev_dropping is zero. Otherwise -the driver should not confirm delivery of the frame and drop it. -Alternatively, the driver can queue the frame internally and call -netif_rx() later when netif_dropping is 0 again. In that case, delivery -confirmation should also be deferred such that the internal queue -cannot grow to much. -This will not reliably avoid packet loss, but the probability -of packet loss in netif_rx() path will be significantly reduced. -(3) Additionally, driver authors might consider to support -CONFIG_NET_HW_FLOWCONTROL. This allows the driver to be woken up -when a previously congested backlog queue becomes empty again. -The driver could uses this for flow-controlling the peer by means -of the LAPB protocol's flow-control service. +Packets should not be reordered or dropped when delivering between the +Packet Layer and the device driver. + +To avoid packets from being reordered or dropped when delivering from +the device driver to the Packet L
[PATCH net-next v3] net: x25: Queue received packets in the drivers instead of per-CPU queues
X.25 Layer 3 (the Packet Layer) expects layer 2 to provide a reliable datalink service such that no packets are reordered or dropped. And X.25 Layer 2 (the LAPB layer) is indeed designed to provide such service. However, this reliability is not preserved when a driver calls "netif_rx" to deliver the received packets to layer 3, because "netif_rx" will put the packets into per-CPU queues before they are delivered to layer 3. If there are multiple CPUs, the order of the packets may not be preserved. The per-CPU queues may also drop packets if there are too many. Therefore, we should not call "netif_rx" to let it queue the packets. Instead, we should use our own queue that won't reorder or drop packets. This patch changes all X.25 drivers to use their own queues instead of calling "netif_rx". The patch also documents this requirement in the "x25-iface" documentation. Cc: Martin Schiller Signed-off-by: Xie He --- Change from v2: No need to print out-of-memory errors when receiving data packets, because the LAPB module is able to handle this situation. (We still need to print the errors when generating control packets.) Change from v1: Drop all PFMEMALLOC skbs because we cannot handle them. Change from RFC (March 5, 2021): Rebased onto the latest net-next. Martin Schiller has acked the RFC version. --- Documentation/networking/x25-iface.rst | 65 -- drivers/net/wan/hdlc_x25.c | 36 +- drivers/net/wan/lapbether.c| 58 +-- 3 files changed, 98 insertions(+), 61 deletions(-) diff --git a/Documentation/networking/x25-iface.rst b/Documentation/networking/x25-iface.rst index df401891dce6..f34e9ec64937 100644 --- a/Documentation/networking/x25-iface.rst +++ b/Documentation/networking/x25-iface.rst @@ -70,60 +70,13 @@ First Byte = 0x03 (X25_IFACE_PARAMS) LAPB parameters. To be defined. +Requirements for the device driver +-- -Possible Problems -= - -(Henner Eisen, 2000-10-28) - -The X.25 packet layer protocol depends on a reliable datalink service. -The LAPB protocol provides such reliable service. But this reliability -is not preserved by the Linux network device driver interface: - -- With Linux 2.4.x (and above) SMP kernels, packet ordering is not - preserved. Even if a device driver calls netif_rx(skb1) and later - netif_rx(skb2), skb2 might be delivered to the network layer - earlier that skb1. -- Data passed upstream by means of netif_rx() might be dropped by the - kernel if the backlog queue is congested. - -The X.25 packet layer protocol will detect this and reset the virtual -call in question. But many upper layer protocols are not designed to -handle such N-Reset events gracefully. And frequent N-Reset events -will always degrade performance. - -Thus, driver authors should make netif_rx() as reliable as possible: - -SMP re-ordering will not occur if the driver's interrupt handler is -always executed on the same CPU. Thus, - -- Driver authors should use irq affinity for the interrupt handler. - -The probability of packet loss due to backlog congestion can be -reduced by the following measures or a combination thereof: - -(1) Drivers for kernel versions 2.4.x and above should always check the -return value of netif_rx(). If it returns NET_RX_DROP, the -driver's LAPB protocol must not confirm reception of the frame -to the peer. -This will reliably suppress packet loss. The LAPB protocol will -automatically cause the peer to re-transmit the dropped packet -later. -The lapb module interface was modified to support this. Its -data_indication() method should now transparently pass the -netif_rx() return value to the (lapb module) caller. -(2) Drivers for kernel versions 2.2.x should always check the global -variable netdev_dropping when a new frame is received. The driver -should only call netif_rx() if netdev_dropping is zero. Otherwise -the driver should not confirm delivery of the frame and drop it. -Alternatively, the driver can queue the frame internally and call -netif_rx() later when netif_dropping is 0 again. In that case, delivery -confirmation should also be deferred such that the internal queue -cannot grow to much. -This will not reliably avoid packet loss, but the probability -of packet loss in netif_rx() path will be significantly reduced. -(3) Additionally, driver authors might consider to support -CONFIG_NET_HW_FLOWCONTROL. This allows the driver to be woken up -when a previously congested backlog queue becomes empty again. -The driver could uses this for flow-controlling the peer by means -of the LAPB protocol's flow-control service. +Packets should not be reordered or dropped when delivering between the +Packet Layer and the device driver. + +To avoid packets from being reordered or dropped when delivering from
[PATCH net-next v2] net: x25: Queue received packets in the drivers instead of per-CPU queues
X.25 Layer 3 (the Packet Layer) expects layer 2 to provide a reliable datalink service such that no packets are reordered or dropped. And X.25 Layer 2 (the LAPB layer) is indeed designed to provide such service. However, this reliability is not preserved when a driver calls "netif_rx" to deliver the received packets to layer 3, because "netif_rx" will put the packets into per-CPU queues before they are delivered to layer 3. If there are multiple CPUs, the order of the packets may not be preserved. The per-CPU queues may also drop packets if there are too many. Therefore, we should not call "netif_rx" to let it queue the packets. Instead, we should use our own queue that won't reorder or drop packets. This patch changes all X.25 drivers to use their own queues instead of calling "netif_rx". The patch also documents this requirement in the "x25-iface" documentation. Cc: Martin Schiller Signed-off-by: Xie He --- Change from v1: Drop all PFMEMALLOC skbs because we cannot handle them. Change from RFC (March 5, 2021): Rebased onto the latest net-next. Martin Schiller has acked the RFC version. --- Documentation/networking/x25-iface.rst | 65 -- drivers/net/wan/hdlc_x25.c | 37 ++- drivers/net/wan/lapbether.c| 59 +-- 3 files changed, 100 insertions(+), 61 deletions(-) diff --git a/Documentation/networking/x25-iface.rst b/Documentation/networking/x25-iface.rst index df401891dce6..f34e9ec64937 100644 --- a/Documentation/networking/x25-iface.rst +++ b/Documentation/networking/x25-iface.rst @@ -70,60 +70,13 @@ First Byte = 0x03 (X25_IFACE_PARAMS) LAPB parameters. To be defined. +Requirements for the device driver +-- -Possible Problems -= - -(Henner Eisen, 2000-10-28) - -The X.25 packet layer protocol depends on a reliable datalink service. -The LAPB protocol provides such reliable service. But this reliability -is not preserved by the Linux network device driver interface: - -- With Linux 2.4.x (and above) SMP kernels, packet ordering is not - preserved. Even if a device driver calls netif_rx(skb1) and later - netif_rx(skb2), skb2 might be delivered to the network layer - earlier that skb1. -- Data passed upstream by means of netif_rx() might be dropped by the - kernel if the backlog queue is congested. - -The X.25 packet layer protocol will detect this and reset the virtual -call in question. But many upper layer protocols are not designed to -handle such N-Reset events gracefully. And frequent N-Reset events -will always degrade performance. - -Thus, driver authors should make netif_rx() as reliable as possible: - -SMP re-ordering will not occur if the driver's interrupt handler is -always executed on the same CPU. Thus, - -- Driver authors should use irq affinity for the interrupt handler. - -The probability of packet loss due to backlog congestion can be -reduced by the following measures or a combination thereof: - -(1) Drivers for kernel versions 2.4.x and above should always check the -return value of netif_rx(). If it returns NET_RX_DROP, the -driver's LAPB protocol must not confirm reception of the frame -to the peer. -This will reliably suppress packet loss. The LAPB protocol will -automatically cause the peer to re-transmit the dropped packet -later. -The lapb module interface was modified to support this. Its -data_indication() method should now transparently pass the -netif_rx() return value to the (lapb module) caller. -(2) Drivers for kernel versions 2.2.x should always check the global -variable netdev_dropping when a new frame is received. The driver -should only call netif_rx() if netdev_dropping is zero. Otherwise -the driver should not confirm delivery of the frame and drop it. -Alternatively, the driver can queue the frame internally and call -netif_rx() later when netif_dropping is 0 again. In that case, delivery -confirmation should also be deferred such that the internal queue -cannot grow to much. -This will not reliably avoid packet loss, but the probability -of packet loss in netif_rx() path will be significantly reduced. -(3) Additionally, driver authors might consider to support -CONFIG_NET_HW_FLOWCONTROL. This allows the driver to be woken up -when a previously congested backlog queue becomes empty again. -The driver could uses this for flow-controlling the peer by means -of the LAPB protocol's flow-control service. +Packets should not be reordered or dropped when delivering between the +Packet Layer and the device driver. + +To avoid packets from being reordered or dropped when delivering from +the device driver to the Packet Layer, the device driver should not +call "netif_rx" to deliver the received packets. Instead, it should +call "netif_receive_skb_core" from softirq cont
[PATCH net-next] net: x25: Queue received packets in the drivers instead of per-CPU queues
X.25 Layer 3 (the Packet Layer) expects layer 2 to provide a reliable datalink service such that no packets are reordered or dropped. And X.25 Layer 2 (the LAPB layer) is indeed designed to provide such service. However, this reliability is not preserved when a driver calls "netif_rx" to deliver the received packets to layer 3, because "netif_rx" will put the packets into per-CPU queues before they are delivered to layer 3. If there are multiple CPUs, the order of the packets may not be preserved. The per-CPU queues may also drop packets if there are too many. Therefore, we should not call "netif_rx" to let it queue the packets. Instead, we should use our own queue that won't reorder or drop packets. This patch changes all X.25 drivers to use their own queues instead of calling "netif_rx". The patch also documents this requirement in the "x25-iface" documentation. Acked-by: Martin Schiller Signed-off-by: Xie He --- Change from RFC (March 5, 2021): Rebased onto the latest net-next (onto the ndo_stop racing fixes for the drivers). --- Documentation/networking/x25-iface.rst | 65 -- drivers/net/wan/hdlc_x25.c | 27 ++- drivers/net/wan/lapbether.c| 44 +++-- 3 files changed, 75 insertions(+), 61 deletions(-) diff --git a/Documentation/networking/x25-iface.rst b/Documentation/networking/x25-iface.rst index df401891dce6..f34e9ec64937 100644 --- a/Documentation/networking/x25-iface.rst +++ b/Documentation/networking/x25-iface.rst @@ -70,60 +70,13 @@ First Byte = 0x03 (X25_IFACE_PARAMS) LAPB parameters. To be defined. +Requirements for the device driver +-- -Possible Problems -= - -(Henner Eisen, 2000-10-28) - -The X.25 packet layer protocol depends on a reliable datalink service. -The LAPB protocol provides such reliable service. But this reliability -is not preserved by the Linux network device driver interface: - -- With Linux 2.4.x (and above) SMP kernels, packet ordering is not - preserved. Even if a device driver calls netif_rx(skb1) and later - netif_rx(skb2), skb2 might be delivered to the network layer - earlier that skb1. -- Data passed upstream by means of netif_rx() might be dropped by the - kernel if the backlog queue is congested. - -The X.25 packet layer protocol will detect this and reset the virtual -call in question. But many upper layer protocols are not designed to -handle such N-Reset events gracefully. And frequent N-Reset events -will always degrade performance. - -Thus, driver authors should make netif_rx() as reliable as possible: - -SMP re-ordering will not occur if the driver's interrupt handler is -always executed on the same CPU. Thus, - -- Driver authors should use irq affinity for the interrupt handler. - -The probability of packet loss due to backlog congestion can be -reduced by the following measures or a combination thereof: - -(1) Drivers for kernel versions 2.4.x and above should always check the -return value of netif_rx(). If it returns NET_RX_DROP, the -driver's LAPB protocol must not confirm reception of the frame -to the peer. -This will reliably suppress packet loss. The LAPB protocol will -automatically cause the peer to re-transmit the dropped packet -later. -The lapb module interface was modified to support this. Its -data_indication() method should now transparently pass the -netif_rx() return value to the (lapb module) caller. -(2) Drivers for kernel versions 2.2.x should always check the global -variable netdev_dropping when a new frame is received. The driver -should only call netif_rx() if netdev_dropping is zero. Otherwise -the driver should not confirm delivery of the frame and drop it. -Alternatively, the driver can queue the frame internally and call -netif_rx() later when netif_dropping is 0 again. In that case, delivery -confirmation should also be deferred such that the internal queue -cannot grow to much. -This will not reliably avoid packet loss, but the probability -of packet loss in netif_rx() path will be significantly reduced. -(3) Additionally, driver authors might consider to support -CONFIG_NET_HW_FLOWCONTROL. This allows the driver to be woken up -when a previously congested backlog queue becomes empty again. -The driver could uses this for flow-controlling the peer by means -of the LAPB protocol's flow-control service. +Packets should not be reordered or dropped when delivering between the +Packet Layer and the device driver. + +To avoid packets from being reordered or dropped when delivering from +the device driver to the Packet Layer, the device driver should not +call "netif_rx" to deliver the received packets. Instead, it should +call "netif_receive_skb_core" from softirq context to deliver them. diff --git a/drivers/net/wan/hdlc_x25.c b/drivers/ne
[PATCH net-next v2] net: lapb: Make "lapb_t1timer_running" able to detect an already running timer
Problem:
The "lapb_t1timer_running" function in "lapb_timer.c" is used in only
one place: in the "lapb_kick" function in "lapb_out.c". "lapb_kick" calls
"lapb_t1timer_running" to check if the timer is already pending, and if
it is not, schedule it to run.
However, if the timer has already fired and is running, and is waiting to
get the "lapb->lock" lock, "lapb_t1timer_running" will not detect this,
and "lapb_kick" will then schedule a new timer. The old timer will then
abort when it sees a new timer pending.
I think this is not right. The purpose of "lapb_kick" should be ensuring
that the actual work of the timer function is scheduled to be done.
If the timer function is already running but waiting for the lock,
"lapb_kick" should not abort and reschedule it.
Changes made:
I added a new field "t1timer_running" in "struct lapb_cb" for
"lapb_t1timer_running" to use. "t1timer_running" will accurately reflect
whether the actual work of the timer is pending. If the timer has fired
but is still waiting for the lock, "t1timer_running" will still correctly
reflect whether the actual work is waiting to be done.
The old "t1timer_stop" field, whose only responsibility is to ask a timer
(that is already running but waiting for the lock) to abort, is no longer
needed, because the new "t1timer_running" field can fully take over its
responsibility. Therefore "t1timer_stop" is deleted.
"t1timer_running" is not simply a negation of the old "t1timer_stop".
At the end of the timer function, if it does not reschedule itself,
"t1timer_running" is set to false to indicate that the timer is stopped.
For consistency of the code, I also added "t2timer_running" and deleted
"t2timer_stop".
Signed-off-by: Xie He
---
Change from v1:
Small improvement to the commit message.
---
include/net/lapb.h| 2 +-
net/lapb/lapb_iface.c | 4 ++--
net/lapb/lapb_timer.c | 19 ---
3 files changed, 15 insertions(+), 10 deletions(-)
diff --git a/include/net/lapb.h b/include/net/lapb.h
index eee73442a1ba..124ee122f2c8 100644
--- a/include/net/lapb.h
+++ b/include/net/lapb.h
@@ -92,7 +92,7 @@ struct lapb_cb {
unsigned short n2, n2count;
unsigned short t1, t2;
struct timer_list t1timer, t2timer;
- boolt1timer_stop, t2timer_stop;
+ boolt1timer_running, t2timer_running;
/* Internal control information */
struct sk_buff_head write_queue;
diff --git a/net/lapb/lapb_iface.c b/net/lapb/lapb_iface.c
index 0511bbe4af7b..1078e14f1acf 100644
--- a/net/lapb/lapb_iface.c
+++ b/net/lapb/lapb_iface.c
@@ -122,8 +122,8 @@ static struct lapb_cb *lapb_create_cb(void)
timer_setup(>t1timer, NULL, 0);
timer_setup(>t2timer, NULL, 0);
- lapb->t1timer_stop = true;
- lapb->t2timer_stop = true;
+ lapb->t1timer_running = false;
+ lapb->t2timer_running = false;
lapb->t1 = LAPB_DEFAULT_T1;
lapb->t2 = LAPB_DEFAULT_T2;
diff --git a/net/lapb/lapb_timer.c b/net/lapb/lapb_timer.c
index 0230b272b7d1..5be68869064d 100644
--- a/net/lapb/lapb_timer.c
+++ b/net/lapb/lapb_timer.c
@@ -40,7 +40,7 @@ void lapb_start_t1timer(struct lapb_cb *lapb)
lapb->t1timer.function = lapb_t1timer_expiry;
lapb->t1timer.expires = jiffies + lapb->t1;
- lapb->t1timer_stop = false;
+ lapb->t1timer_running = true;
add_timer(>t1timer);
}
@@ -51,25 +51,25 @@ void lapb_start_t2timer(struct lapb_cb *lapb)
lapb->t2timer.function = lapb_t2timer_expiry;
lapb->t2timer.expires = jiffies + lapb->t2;
- lapb->t2timer_stop = false;
+ lapb->t2timer_running = true;
add_timer(>t2timer);
}
void lapb_stop_t1timer(struct lapb_cb *lapb)
{
- lapb->t1timer_stop = true;
+ lapb->t1timer_running = false;
del_timer(>t1timer);
}
void lapb_stop_t2timer(struct lapb_cb *lapb)
{
- lapb->t2timer_stop = true;
+ lapb->t2timer_running = false;
del_timer(>t2timer);
}
int lapb_t1timer_running(struct lapb_cb *lapb)
{
- return timer_pending(>t1timer);
+ return lapb->t1timer_running;
}
static void lapb_t2timer_expiry(struct timer_list *t)
@@ -79,13 +79,14 @@ static void lapb_t2timer_expiry(struct timer_list *t)
spin_lock_bh(>lock);
if (timer_pending(>t2timer)) /* A new timer has been set up */
goto out;
- if (lapb->t2timer_stop) /* The timer has been stopped */
+ if (!lapb->t2timer_running) /* The timer has been stopped */
goto out;
if (lapb->condition & LAPB_ACK_PENDING
[PATCH net-next] net: lapb: Make "lapb_t1timer_running" able to detect an already running timer
Problem:
The "lapb_t1timer_running" function in "lapb_timer.c" is used in only
one place: in the "lapb_kick" function in "lapb_out.c". "lapb_kick" calls
"lapb_t1timer_running" to check if the timer is already pending, and if
it is not, schedule it to run.
However, if the timer has already fired and is running, and is waiting to
get the "lapb->lock" lock, "lapb_t1timer_running" will not detect this,
and "lapb_kick" will then schedule a new timer, which causes the old
timer to be aborted.
I think this is not right. The purpose of "lapb_kick" should be ensuring
that the actual work of the timer function is scheduled to be done.
If the timer function is already running but waiting for the lock,
"lapb_kick" should not abort and reschedule it.
Changes made:
I added a new field "t1timer_running" in "struct lapb_cb" for
"lapb_t1timer_running" to use. "t1timer_running" will accurately reflect
whether the actual work of the timer is pending. If the timer has fired
but is still waiting for the lock, "t1timer_running" will still correctly
reflect whether the actual work is waiting to be done.
The old "t1timer_stop" field, whose only responsibility is to ask a timer
(that is already running but waiting for the lock) to abort, is no longer
needed, because the new "t1timer_running" field can fully take over its
responsibility. Therefore "t1timer_stop" is deleted.
"t1timer_running" is not simply a negation of the old "t1timer_stop".
At the end of the timer function, if it does not reschedule itself,
"t1timer_running" is set to false to indicate that the timer is stopped.
For consistency of the code, I also added "t2timer_running" and deleted
"t2timer_stop".
Signed-off-by: Xie He
---
include/net/lapb.h| 2 +-
net/lapb/lapb_iface.c | 4 ++--
net/lapb/lapb_timer.c | 19 ---
3 files changed, 15 insertions(+), 10 deletions(-)
diff --git a/include/net/lapb.h b/include/net/lapb.h
index eee73442a1ba..124ee122f2c8 100644
--- a/include/net/lapb.h
+++ b/include/net/lapb.h
@@ -92,7 +92,7 @@ struct lapb_cb {
unsigned short n2, n2count;
unsigned short t1, t2;
struct timer_list t1timer, t2timer;
- boolt1timer_stop, t2timer_stop;
+ boolt1timer_running, t2timer_running;
/* Internal control information */
struct sk_buff_head write_queue;
diff --git a/net/lapb/lapb_iface.c b/net/lapb/lapb_iface.c
index 0511bbe4af7b..1078e14f1acf 100644
--- a/net/lapb/lapb_iface.c
+++ b/net/lapb/lapb_iface.c
@@ -122,8 +122,8 @@ static struct lapb_cb *lapb_create_cb(void)
timer_setup(>t1timer, NULL, 0);
timer_setup(>t2timer, NULL, 0);
- lapb->t1timer_stop = true;
- lapb->t2timer_stop = true;
+ lapb->t1timer_running = false;
+ lapb->t2timer_running = false;
lapb->t1 = LAPB_DEFAULT_T1;
lapb->t2 = LAPB_DEFAULT_T2;
diff --git a/net/lapb/lapb_timer.c b/net/lapb/lapb_timer.c
index 0230b272b7d1..5be68869064d 100644
--- a/net/lapb/lapb_timer.c
+++ b/net/lapb/lapb_timer.c
@@ -40,7 +40,7 @@ void lapb_start_t1timer(struct lapb_cb *lapb)
lapb->t1timer.function = lapb_t1timer_expiry;
lapb->t1timer.expires = jiffies + lapb->t1;
- lapb->t1timer_stop = false;
+ lapb->t1timer_running = true;
add_timer(>t1timer);
}
@@ -51,25 +51,25 @@ void lapb_start_t2timer(struct lapb_cb *lapb)
lapb->t2timer.function = lapb_t2timer_expiry;
lapb->t2timer.expires = jiffies + lapb->t2;
- lapb->t2timer_stop = false;
+ lapb->t2timer_running = true;
add_timer(>t2timer);
}
void lapb_stop_t1timer(struct lapb_cb *lapb)
{
- lapb->t1timer_stop = true;
+ lapb->t1timer_running = false;
del_timer(>t1timer);
}
void lapb_stop_t2timer(struct lapb_cb *lapb)
{
- lapb->t2timer_stop = true;
+ lapb->t2timer_running = false;
del_timer(>t2timer);
}
int lapb_t1timer_running(struct lapb_cb *lapb)
{
- return timer_pending(>t1timer);
+ return lapb->t1timer_running;
}
static void lapb_t2timer_expiry(struct timer_list *t)
@@ -79,13 +79,14 @@ static void lapb_t2timer_expiry(struct timer_list *t)
spin_lock_bh(>lock);
if (timer_pending(>t2timer)) /* A new timer has been set up */
goto out;
- if (lapb->t2timer_stop) /* The timer has been stopped */
+ if (!lapb->t2timer_running) /* The timer has been stopped */
goto out;
if (lapb->condition & LAPB_ACK_PENDING_CONDITION) {
lapb->condition &= ~LAPB_ACK_PENDING_CONDI
[PATCH net-next] net: lapbether: Close the LAPB device before its underlying Ethernet device closes
When a virtual LAPB device's underlying Ethernet device closes, the LAPB device is also closed. However, currently the LAPB device is closed after the Ethernet device closes. It would be better to close it before the Ethernet device closes. This would allow the LAPB device to transmit a last frame to notify the other side that it is disconnecting. Signed-off-by: Xie He --- drivers/net/wan/lapbether.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/net/wan/lapbether.c b/drivers/net/wan/lapbether.c index 8fda0446ff71..45d74285265a 100644 --- a/drivers/net/wan/lapbether.c +++ b/drivers/net/wan/lapbether.c @@ -421,8 +421,8 @@ static int lapbeth_device_event(struct notifier_block *this, if (lapbeth_get_x25_dev(dev) == NULL) lapbeth_new_device(dev); break; - case NETDEV_DOWN: - /* ethernet device closed -> close LAPB interface */ + case NETDEV_GOING_DOWN: + /* ethernet device closes -> close LAPB interface */ lapbeth = lapbeth_get_x25_dev(dev); if (lapbeth) dev_close(lapbeth->axdev); -- 2.27.0
Re: [PATCH net] net: lapbether: Prevent racing when checking whether the netif is running
On Tue, Mar 16, 2021 at 8:17 AM Martin Schiller wrote: > > On 2021-03-14 02:59, Xie He wrote: > > Hi Martin, > > > > Could you ack? Thanks! > > Acked-by: Martin Schiller Thank you!!
[PATCH net v2] net: hdlc_x25: Prevent racing between "x25_close" and "x25_xmit"/"x25_rx"
"x25_close" is called by "hdlc_close" in "hdlc.c", which is called by
hardware drivers' "ndo_stop" function.
"x25_xmit" is called by "hdlc_start_xmit" in "hdlc.c", which is hardware
drivers' "ndo_start_xmit" function.
"x25_rx" is called by "hdlc_rcv" in "hdlc.c", which receives HDLC frames
from "net/core/dev.c".
"x25_close" races with "x25_xmit" and "x25_rx" because their callers race.
However, we need to ensure that the LAPB APIs called in "x25_xmit" and
"x25_rx" are called before "lapb_unregister" is called in "x25_close".
This patch adds locking to ensure when "x25_xmit" and "x25_rx" are doing
their work, "lapb_unregister" is not yet called in "x25_close".
Reasons for not solving the racing between "x25_close" and "x25_xmit" by
calling "netif_tx_disable" in "x25_close":
1. We still need to solve the racing between "x25_close" and "x25_rx";
2. The design of the HDLC subsystem assumes the HDLC hardware drivers
have full control over the TX queue, and the HDLC protocol drivers (like
this driver) have no control. Controlling the queue here in the protocol
driver may interfere with hardware drivers' control of the queue.
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Xie He
---
Change from v1:
Added a "Fixes" tag.
---
drivers/net/wan/hdlc_x25.c | 42 +-
1 file changed, 41 insertions(+), 1 deletion(-)
diff --git a/drivers/net/wan/hdlc_x25.c b/drivers/net/wan/hdlc_x25.c
index 4aaa6388b9ee..5a6a945f6c81 100644
--- a/drivers/net/wan/hdlc_x25.c
+++ b/drivers/net/wan/hdlc_x25.c
@@ -23,6 +23,8 @@
struct x25_state {
x25_hdlc_proto settings;
+ bool up;
+ spinlock_t up_lock; /* Protects "up" */
};
static int x25_ioctl(struct net_device *dev, struct ifreq *ifr);
@@ -104,6 +106,8 @@ static void x25_data_transmit(struct net_device *dev,
struct sk_buff *skb)
static netdev_tx_t x25_xmit(struct sk_buff *skb, struct net_device *dev)
{
+ hdlc_device *hdlc = dev_to_hdlc(dev);
+ struct x25_state *x25st = state(hdlc);
int result;
/* There should be a pseudo header of 1 byte added by upper layers.
@@ -114,11 +118,19 @@ static netdev_tx_t x25_xmit(struct sk_buff *skb, struct
net_device *dev)
return NETDEV_TX_OK;
}
+ spin_lock_bh(>up_lock);
+ if (!x25st->up) {
+ spin_unlock_bh(>up_lock);
+ kfree_skb(skb);
+ return NETDEV_TX_OK;
+ }
+
switch (skb->data[0]) {
case X25_IFACE_DATA:/* Data to be transmitted */
skb_pull(skb, 1);
if ((result = lapb_data_request(dev, skb)) != LAPB_OK)
dev_kfree_skb(skb);
+ spin_unlock_bh(>up_lock);
return NETDEV_TX_OK;
case X25_IFACE_CONNECT:
@@ -147,6 +159,7 @@ static netdev_tx_t x25_xmit(struct sk_buff *skb, struct
net_device *dev)
break;
}
+ spin_unlock_bh(>up_lock);
dev_kfree_skb(skb);
return NETDEV_TX_OK;
}
@@ -164,6 +177,7 @@ static int x25_open(struct net_device *dev)
.data_transmit = x25_data_transmit,
};
hdlc_device *hdlc = dev_to_hdlc(dev);
+ struct x25_state *x25st = state(hdlc);
struct lapb_parms_struct params;
int result;
@@ -190,6 +204,10 @@ static int x25_open(struct net_device *dev)
if (result != LAPB_OK)
return -EINVAL;
+ spin_lock_bh(>up_lock);
+ x25st->up = true;
+ spin_unlock_bh(>up_lock);
+
return 0;
}
@@ -197,6 +215,13 @@ static int x25_open(struct net_device *dev)
static void x25_close(struct net_device *dev)
{
+ hdlc_device *hdlc = dev_to_hdlc(dev);
+ struct x25_state *x25st = state(hdlc);
+
+ spin_lock_bh(>up_lock);
+ x25st->up = false;
+ spin_unlock_bh(>up_lock);
+
lapb_unregister(dev);
}
@@ -205,15 +230,28 @@ static void x25_close(struct net_device *dev)
static int x25_rx(struct sk_buff *skb)
{
struct net_device *dev = skb->dev;
+ hdlc_device *hdlc = dev_to_hdlc(dev);
+ struct x25_state *x25st = state(hdlc);
if ((skb = skb_share_check(skb, GFP_ATOMIC)) == NULL) {
dev->stats.rx_dropped++;
return NET_RX_DROP;
}
- if (lapb_data_received(dev, skb) == LAPB_OK)
+ spin_lock_bh(>up_lock);
+ if (!x25st->up) {
+ spin_unlock_bh(>up_lock);
+ kfree_skb(skb);
+ dev->stats.rx_dropped++;
+ return NET_RX_DROP;
+ }
+
+ if (lapb_data_
[PATCH net] net: hdlc_x25: Prevent racing between "x25_close" and "x25_xmit"/"x25_rx"
"x25_close" is called by "hdlc_close" in "hdlc.c", which is called by
hardware drivers' "ndo_stop" function.
"x25_xmit" is called by "hdlc_start_xmit" in "hdlc.c", which is hardware
drivers' "ndo_start_xmit" function.
"x25_rx" is called by "hdlc_rcv" in "hdlc.c", which receives HDLC frames
from "net/core/dev.c".
"x25_close" races with "x25_xmit" and "x25_rx" because their callers race.
However, we need to ensure that the LAPB APIs called in "x25_xmit" and
"x25_rx" are called before "lapb_unregister" is called in "x25_close".
This patch adds locking to ensure when "x25_xmit" and "x25_rx" are doing
their work, "lapb_unregister" is not yet called in "x25_close".
Reasons for not solving the racing between "x25_close" and "x25_xmit" by
calling "netif_tx_disable" in "x25_close":
1. We still need to solve the racing between "x25_close" and "x25_rx";
2. The design of the HDLC subsystem assumes the HDLC hardware drivers
have full control over the TX queue, and the HDLC protocol drivers (like
this driver) have no control. Controlling the queue here in the protocol
driver may interfere with hardware drivers' control of the queue.
Signed-off-by: Xie He
---
drivers/net/wan/hdlc_x25.c | 42 +-
1 file changed, 41 insertions(+), 1 deletion(-)
diff --git a/drivers/net/wan/hdlc_x25.c b/drivers/net/wan/hdlc_x25.c
index 4aaa6388b9ee..5a6a945f6c81 100644
--- a/drivers/net/wan/hdlc_x25.c
+++ b/drivers/net/wan/hdlc_x25.c
@@ -23,6 +23,8 @@
struct x25_state {
x25_hdlc_proto settings;
+ bool up;
+ spinlock_t up_lock; /* Protects "up" */
};
static int x25_ioctl(struct net_device *dev, struct ifreq *ifr);
@@ -104,6 +106,8 @@ static void x25_data_transmit(struct net_device *dev,
struct sk_buff *skb)
static netdev_tx_t x25_xmit(struct sk_buff *skb, struct net_device *dev)
{
+ hdlc_device *hdlc = dev_to_hdlc(dev);
+ struct x25_state *x25st = state(hdlc);
int result;
/* There should be a pseudo header of 1 byte added by upper layers.
@@ -114,11 +118,19 @@ static netdev_tx_t x25_xmit(struct sk_buff *skb, struct
net_device *dev)
return NETDEV_TX_OK;
}
+ spin_lock_bh(>up_lock);
+ if (!x25st->up) {
+ spin_unlock_bh(>up_lock);
+ kfree_skb(skb);
+ return NETDEV_TX_OK;
+ }
+
switch (skb->data[0]) {
case X25_IFACE_DATA:/* Data to be transmitted */
skb_pull(skb, 1);
if ((result = lapb_data_request(dev, skb)) != LAPB_OK)
dev_kfree_skb(skb);
+ spin_unlock_bh(>up_lock);
return NETDEV_TX_OK;
case X25_IFACE_CONNECT:
@@ -147,6 +159,7 @@ static netdev_tx_t x25_xmit(struct sk_buff *skb, struct
net_device *dev)
break;
}
+ spin_unlock_bh(>up_lock);
dev_kfree_skb(skb);
return NETDEV_TX_OK;
}
@@ -164,6 +177,7 @@ static int x25_open(struct net_device *dev)
.data_transmit = x25_data_transmit,
};
hdlc_device *hdlc = dev_to_hdlc(dev);
+ struct x25_state *x25st = state(hdlc);
struct lapb_parms_struct params;
int result;
@@ -190,6 +204,10 @@ static int x25_open(struct net_device *dev)
if (result != LAPB_OK)
return -EINVAL;
+ spin_lock_bh(>up_lock);
+ x25st->up = true;
+ spin_unlock_bh(>up_lock);
+
return 0;
}
@@ -197,6 +215,13 @@ static int x25_open(struct net_device *dev)
static void x25_close(struct net_device *dev)
{
+ hdlc_device *hdlc = dev_to_hdlc(dev);
+ struct x25_state *x25st = state(hdlc);
+
+ spin_lock_bh(>up_lock);
+ x25st->up = false;
+ spin_unlock_bh(>up_lock);
+
lapb_unregister(dev);
}
@@ -205,15 +230,28 @@ static void x25_close(struct net_device *dev)
static int x25_rx(struct sk_buff *skb)
{
struct net_device *dev = skb->dev;
+ hdlc_device *hdlc = dev_to_hdlc(dev);
+ struct x25_state *x25st = state(hdlc);
if ((skb = skb_share_check(skb, GFP_ATOMIC)) == NULL) {
dev->stats.rx_dropped++;
return NET_RX_DROP;
}
- if (lapb_data_received(dev, skb) == LAPB_OK)
+ spin_lock_bh(>up_lock);
+ if (!x25st->up) {
+ spin_unlock_bh(>up_lock);
+ kfree_skb(skb);
+ dev->stats.rx_dropped++;
+ return NET_RX_DROP;
+ }
+
+ if (lapb_data_received(dev, skb) == LAPB_OK) {
+ spin_unlock_bh(>up_lock);
return NET_RX_SUCCESS;
+
Re: [PATCH net] net: lapbether: Prevent racing when checking whether the netif is running
Hi Martin, Could you ack? Thanks!
Re: [PATCH] net: socket.c: Fix comparison issues
What is the reason for this change? Why is the new way better than the old way?
Re: [PATCH net] net: lapbether: Prevent racing when checking whether the netif is running
On Thu, Mar 11, 2021 at 4:10 PM Jakub Kicinski wrote: > > And the "noqueue" queue is there because it's on top of hdlc_fr.c > somehow or some out of tree driver? Or do you install it manually? No, this driver is not related to "hdlc_fr.c" or any out-of-tree driver. The default qdisc is "noqueue" for this driver because this driver doesn't set "tx_queue_len". This means the value of "tx_queue_len" would be 0. In this case, "alloc_netdev_mqs" will automatically add the "IFF_NO_QUEUE" flag to the device, then "attach_one_default_qdisc" in "net/sched/sch_generic.c" will attach the "noqueue" qdisc for devices with the "IFF_NO_QUEUE" flag.
Re: [PATCH net] net: lapbether: Prevent racing when checking whether the netif is running
On Thu, Mar 11, 2021 at 2:52 PM Jakub Kicinski wrote:
>
> Normally driver's ndo_stop() calls netif_tx_disable() which takes TX
> locks, so unless your driver is lockless (LLTX) there should be no xmit
> calls after that point.
Do you mean I should call "netif_tx_disable" inside my "ndo_stop"
function as a fix for the racing between "ndo_stop" and
"ndo_start_xmit"?
I can't call "netif_tx_disable" inside my "ndo_stop" function because
"netif_tx_disable" will call "netif_tx_stop_queue", which causes
another racing problem. Please see my recent commit f7d9d4854519
("net: lapbether: Remove netif_start_queue / netif_stop_queue")
Re: [PATCH net] net: lapbether: Prevent racing when checking whether the netif is running
On Thu, Mar 11, 2021 at 12:43 PM Jakub Kicinski wrote: > > Is this a theoretical issues or do you see a path where it triggers? > > Who are the callers sending frames to a device which went down? This is a theoretical issue. I didn't see this issue in practice. When "__dev_queue_xmit" and "sch_direct_xmit" call "dev_hard_start_xmit", there appears to be no locking mechanism preventing the netif from going down while "dev_hard_start_xmit" is doing its work. David once confirmed in an email that a driver's "ndo_stop" function would indeed race with its "ndo_start_xmit" function: https://lore.kernel.org/netdev/20190520.200922.2277656639346033061.da...@davemloft.net/
[PATCH net] net: lapbether: Prevent racing when checking whether the netif is running
There are two "netif_running" checks in this driver. One is in
"lapbeth_xmit" and the other is in "lapbeth_rcv". They serve to make
sure that the LAPB APIs called in these functions are called before
"lapb_unregister" is called by the "ndo_stop" function.
However, these "netif_running" checks are unreliable, because it's
possible that immediately after "netif_running" returns true, "ndo_stop"
is called (which causes "lapb_unregister" to be called).
This patch adds locking to make sure "lapbeth_xmit" and "lapbeth_rcv" can
reliably check and ensure the netif is running while doing their work.
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Xie He
---
drivers/net/wan/lapbether.c | 32 +---
1 file changed, 25 insertions(+), 7 deletions(-)
diff --git a/drivers/net/wan/lapbether.c b/drivers/net/wan/lapbether.c
index c3372498f4f1..8fda0446ff71 100644
--- a/drivers/net/wan/lapbether.c
+++ b/drivers/net/wan/lapbether.c
@@ -51,6 +51,8 @@ struct lapbethdev {
struct list_headnode;
struct net_device *ethdev;/* link to ethernet device */
struct net_device *axdev; /* lapbeth device (lapb#) */
+ boolup;
+ spinlock_t up_lock;/* Protects "up" */
};
static LIST_HEAD(lapbeth_devices);
@@ -101,8 +103,9 @@ static int lapbeth_rcv(struct sk_buff *skb, struct
net_device *dev, struct packe
rcu_read_lock();
lapbeth = lapbeth_get_x25_dev(dev);
if (!lapbeth)
- goto drop_unlock;
- if (!netif_running(lapbeth->axdev))
+ goto drop_unlock_rcu;
+ spin_lock_bh(>up_lock);
+ if (!lapbeth->up)
goto drop_unlock;
len = skb->data[0] + skb->data[1] * 256;
@@ -117,11 +120,14 @@ static int lapbeth_rcv(struct sk_buff *skb, struct
net_device *dev, struct packe
goto drop_unlock;
}
out:
+ spin_unlock_bh(>up_lock);
rcu_read_unlock();
return 0;
drop_unlock:
kfree_skb(skb);
goto out;
+drop_unlock_rcu:
+ rcu_read_unlock();
drop:
kfree_skb(skb);
return 0;
@@ -151,13 +157,11 @@ static int lapbeth_data_indication(struct net_device
*dev, struct sk_buff *skb)
static netdev_tx_t lapbeth_xmit(struct sk_buff *skb,
struct net_device *dev)
{
+ struct lapbethdev *lapbeth = netdev_priv(dev);
int err;
- /*
-* Just to be *really* sure not to send anything if the interface
-* is down, the ethernet device may have gone.
-*/
- if (!netif_running(dev))
+ spin_lock_bh(>up_lock);
+ if (!lapbeth->up)
goto drop;
/* There should be a pseudo header of 1 byte added by upper layers.
@@ -194,6 +198,7 @@ static netdev_tx_t lapbeth_xmit(struct sk_buff *skb,
goto drop;
}
out:
+ spin_unlock_bh(>up_lock);
return NETDEV_TX_OK;
drop:
kfree_skb(skb);
@@ -285,6 +290,7 @@ static const struct lapb_register_struct lapbeth_callbacks
= {
*/
static int lapbeth_open(struct net_device *dev)
{
+ struct lapbethdev *lapbeth = netdev_priv(dev);
int err;
if ((err = lapb_register(dev, _callbacks)) != LAPB_OK) {
@@ -292,13 +298,22 @@ static int lapbeth_open(struct net_device *dev)
return -ENODEV;
}
+ spin_lock_bh(>up_lock);
+ lapbeth->up = true;
+ spin_unlock_bh(>up_lock);
+
return 0;
}
static int lapbeth_close(struct net_device *dev)
{
+ struct lapbethdev *lapbeth = netdev_priv(dev);
int err;
+ spin_lock_bh(>up_lock);
+ lapbeth->up = false;
+ spin_unlock_bh(>up_lock);
+
if ((err = lapb_unregister(dev)) != LAPB_OK)
pr_err("lapb_unregister error: %d\n", err);
@@ -356,6 +371,9 @@ static int lapbeth_new_device(struct net_device *dev)
dev_hold(dev);
lapbeth->ethdev = dev;
+ lapbeth->up = false;
+ spin_lock_init(>up_lock);
+
rc = -EIO;
if (register_netdevice(ndev))
goto fail;
--
2.27.0
Re: [PATCH net-next RFC] net: x25: Queue received packets in the drivers instead of per-CPU queues
On Tue, Mar 9, 2021 at 5:23 AM Martin Schiller wrote: > > I've tested the hdlc_x25 driver. > Looks good to me. > > Acked-by: Martin Schiller Thank you!! I'll re-send this patch after net-next is re-opened and my other fixes get merged into net-next. Thanks!
[PATCH net] net: lapbether: Remove netif_start_queue / netif_stop_queue
For the devices in this driver, the default qdisc is "noqueue",
because their "tx_queue_len" is 0.
In function "__dev_queue_xmit" in "net/core/dev.c", devices with the
"noqueue" qdisc are specially handled. Packets are transmitted without
being queued after a "dev->flags & IFF_UP" check. However, it's possible
that even if this check succeeds, "ops->ndo_stop" may still have already
been called. This is because in "__dev_close_many", "ops->ndo_stop" is
called before clearing the "IFF_UP" flag.
If we call "netif_stop_queue" in "ops->ndo_stop", then it's possible in
"__dev_queue_xmit", it sees the "IFF_UP" flag is present, and then it
checks "netif_xmit_stopped" and finds that the queue is already stopped.
In this case, it will complain that:
"Virtual device ... asks to queue packet!"
To prevent "__dev_queue_xmit" from generating this complaint, we should
not call "netif_stop_queue" in "ops->ndo_stop".
We also don't need to call "netif_start_queue" in "ops->ndo_open",
because after a netdev is allocated and registered, the
"__QUEUE_STATE_DRV_XOFF" flag is initially not set, so there is no need
to call "netif_start_queue" to clear it.
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Xie He
---
drivers/net/wan/lapbether.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/net/wan/lapbether.c b/drivers/net/wan/lapbether.c
index 605fe555e157..c3372498f4f1 100644
--- a/drivers/net/wan/lapbether.c
+++ b/drivers/net/wan/lapbether.c
@@ -292,7 +292,6 @@ static int lapbeth_open(struct net_device *dev)
return -ENODEV;
}
- netif_start_queue(dev);
return 0;
}
@@ -300,8 +299,6 @@ static int lapbeth_close(struct net_device *dev)
{
int err;
- netif_stop_queue(dev);
-
if ((err = lapb_unregister(dev)) != LAPB_OK)
pr_err("lapb_unregister error: %d\n", err);
--
2.27.0
[PATCH net-next RFC] net: x25: Queue received packets in the drivers instead of per-CPU queues
X.25 Layer 3 (the Packet Layer) expects layer 2 to provide a reliable datalink service such that no packets are reordered or dropped. And X.25 Layer 2 (the LAPB layer) is indeed designed to provide such service. However, this reliability is not preserved when a driver calls "netif_rx" to deliver the received packets to layer 3, because "netif_rx" will put the packets into per-CPU queues before they are delivered to layer 3. If there are multiple CPUs, the order of the packets may not be preserved. The per-CPU queues may also drop packets if there are too many. Therefore, we should not call "netif_rx" to let it queue the packets. Instead, we should use our own queue that won't reorder or drop packets. This patch changes all X.25 drivers to use their own queues instead of calling "netif_rx". The patch also documents this requirement in the "x25-iface" documentation. Cc: Martin Schiller Signed-off-by: Xie He --- Documentation/networking/x25-iface.rst | 65 -- drivers/net/wan/hdlc_x25.c | 29 +++- drivers/net/wan/lapbether.c| 46 -- 3 files changed, 79 insertions(+), 61 deletions(-) diff --git a/Documentation/networking/x25-iface.rst b/Documentation/networking/x25-iface.rst index df401891dce6..f34e9ec64937 100644 --- a/Documentation/networking/x25-iface.rst +++ b/Documentation/networking/x25-iface.rst @@ -70,60 +70,13 @@ First Byte = 0x03 (X25_IFACE_PARAMS) LAPB parameters. To be defined. +Requirements for the device driver +-- -Possible Problems -= - -(Henner Eisen, 2000-10-28) - -The X.25 packet layer protocol depends on a reliable datalink service. -The LAPB protocol provides such reliable service. But this reliability -is not preserved by the Linux network device driver interface: - -- With Linux 2.4.x (and above) SMP kernels, packet ordering is not - preserved. Even if a device driver calls netif_rx(skb1) and later - netif_rx(skb2), skb2 might be delivered to the network layer - earlier that skb1. -- Data passed upstream by means of netif_rx() might be dropped by the - kernel if the backlog queue is congested. - -The X.25 packet layer protocol will detect this and reset the virtual -call in question. But many upper layer protocols are not designed to -handle such N-Reset events gracefully. And frequent N-Reset events -will always degrade performance. - -Thus, driver authors should make netif_rx() as reliable as possible: - -SMP re-ordering will not occur if the driver's interrupt handler is -always executed on the same CPU. Thus, - -- Driver authors should use irq affinity for the interrupt handler. - -The probability of packet loss due to backlog congestion can be -reduced by the following measures or a combination thereof: - -(1) Drivers for kernel versions 2.4.x and above should always check the -return value of netif_rx(). If it returns NET_RX_DROP, the -driver's LAPB protocol must not confirm reception of the frame -to the peer. -This will reliably suppress packet loss. The LAPB protocol will -automatically cause the peer to re-transmit the dropped packet -later. -The lapb module interface was modified to support this. Its -data_indication() method should now transparently pass the -netif_rx() return value to the (lapb module) caller. -(2) Drivers for kernel versions 2.2.x should always check the global -variable netdev_dropping when a new frame is received. The driver -should only call netif_rx() if netdev_dropping is zero. Otherwise -the driver should not confirm delivery of the frame and drop it. -Alternatively, the driver can queue the frame internally and call -netif_rx() later when netif_dropping is 0 again. In that case, delivery -confirmation should also be deferred such that the internal queue -cannot grow to much. -This will not reliably avoid packet loss, but the probability -of packet loss in netif_rx() path will be significantly reduced. -(3) Additionally, driver authors might consider to support -CONFIG_NET_HW_FLOWCONTROL. This allows the driver to be woken up -when a previously congested backlog queue becomes empty again. -The driver could uses this for flow-controlling the peer by means -of the LAPB protocol's flow-control service. +Packets should not be reordered or dropped when delivering between the +Packet Layer and the device driver. + +To avoid packets from being reordered or dropped when delivering from +the device driver to the Packet Layer, the device driver should not +call "netif_rx" to deliver the received packets. Instead, it should +call "netif_receive_skb_core" from softirq context to deliver them. diff --git a/drivers/net/wan/hdlc_x25.c b/drivers/net/wan/hdlc_x25.c index 4aaa6388b9ee..28e9cb2c5f1e 100644 --- a/drivers/net/wan/hdlc_x25.c +++ b/drivers/net/wan/hdlc_x25.c @
Re: [PATCH net-next RFC v4] net: hdlc_x25: Queue outgoing LAPB frames
On Wed, Mar 3, 2021 at 5:26 AM Martin Schiller wrote: > > On 2021-03-03 00:30, Jakub Kicinski wrote: > > > > Hard question to answer, existing users seem happy and Xie's driver > > isn't upstream, so the justification for potentially breaking backward > > compatibility isn't exactly "strong". > > > > Can we cop out and add a knob somewhere to control spawning the extra > > netdev? Let people who just want a newer kernel carry on without > > distractions and those who want the extra layer can flip the switch? > > Yes, that would be a good compromise. > I think a compile time selection option is enough here. > We could introduce a new config option CONFIG_HDLC_X25_LEGACY (or > something like that) and then implement the new or the old behavior in > the driver accordingly. > > A switch that can be toggled at runtime (e.g. via sethdlc) would also be > conceivable, but I don't think this is necessary. Yes, I think adding a new config option would be a good way. Thank you both!!
Re: [PATCH net-next RFC v4] net: hdlc_x25: Queue outgoing LAPB frames
On Sun, Feb 28, 2021 at 10:56 PM Martin Schiller wrote: > > >> Also, I have a hard time assessing if such a wrap is really > >> enforceable. > > > > Sorry. I don't understand what you mean. What "wrap" are you referring > > to? > > I mean the change from only one hdlc interface to both hdlc and > hdlc_x25. > > I can't estimate how many users are out there and how their setup looks > like. I'm also thinking about solving this issue by adding new APIs to the HDLC subsystem (hdlc_stop_queue / hdlc_wake_queue) for hardware drivers to call instead of netif_stop_queue / netif_wake_queue. This way we can preserve backward compatibility. However I'm reluctant to change the code of all the hardware drivers because I'm afraid of introducing bugs, etc. When I look at the code of "wan/lmc/lmc_main.c", I feel I'm not able to make sure there are no bugs (related to stop_queue / wake_queue) after my change (and even before my change, actually). There are even serious style problems: the majority of its lines are indented by spaces. So I don't want to mess with all the hardware drivers. Hardware driver developers (if they wish to properly support hdlc_x25) should do the change themselves. This is not a problem for me, because I use my own out-of-tree hardware driver. However if I add APIs with no user code in the kernel, other developers may think these APIs are not necessary.
Re: [PATCH net-next RFC v4] net: hdlc_x25: Queue outgoing LAPB frames
On Fri, Feb 26, 2021 at 6:21 AM Martin Schiller wrote: > > I have now had a look at it. It works as expected. > I just wonder if it would not be more appropriate to call > the lapb_register() already in x25_hdlc_open(), so that the layer2 > (lapb) can already "work" before the hdlc_x25 interface is up. I think it's better not to keep LAPB running unless hdlc_x25 is up. If I am the user, I would expect that when I change the X.25 interface to the DOWN state, the LAPB protocol would be completely stopped and the LAPB layer would not generate any new frames anymore (even if the other side wants to connect), and when I change the X.25 interface back to the UP state, it would be a fresh new start for the LAPB protocol. > Also, I have a hard time assessing if such a wrap is really enforceable. Sorry. I don't understand what you mean. What "wrap" are you referring to? > Unfortunately I have no idea how many users there actually are.
Re: [PATCH net-next RFC v4] net: hdlc_x25: Queue outgoing LAPB frames
On Sun, Feb 21, 2021 at 11:14 PM Martin Schiller wrote: > > I'm not really happy with this change because it breaks compatibility. > We then suddenly have 2 interfaces; the X.25 routings are to be set via > the "new" hdlc_x25 interfaces instead of the hdlc interfaces. > > I currently just don't have a nicer solution to fix this queueing > problem either. On the other hand, since the many years we have been > using the current state, I have never noticed any problems with > discarded frames. So it might be more a theoretical problem than a > practical one. This problem becomes very serious when we use AF_PACKET sockets, because the majority of frames would be dropped by the hardware driver, which significantly impacts transmission speed. What I am really doing is to enable adequate support for AF_PACKET sockets, allowing users to use the bare (raw) LAPB protocol. If we take this into consideration, this problem is no longer just a theoretical problem, but a real practical issue. If we don't want to break backward compatibility, there is another option: We can create a new API for the HDLC subsystem for stopping/restarting the TX queue, and replace all HDLC hardware drivers' netif_stop_queue and netif_wake_queue calls with calls to this new API. This new API would then call hdlc_x25 to stop/restart its internal queue. But this option would require modifying all HDLC hardware drivers' code, and frankly, not all HDLC hardware drivers' developers care about running X.25 protocols on their hardware. So this would cause both hardware driver instabilities and confusion for hardware driver developers.
Re: [PATCH net-next RFC v4] net: hdlc_x25: Queue outgoing LAPB frames
On Sat, Feb 20, 2021 at 10:39 PM Leon Romanovsky wrote: > > > Yes, this patch will break backward compatibility. Users with old > > scripts will find them no longer working. > > Did you search in debian/fedora code repositories to see if such scripts exist > as part of any distro package? I just tried to search in Debian and Fedora packages but didn't seem to find anything related. I searched "hdlc" and "x25". The only things I can find are "AX.25" related things. But "AX.25" is not related to "X.25". I guess a script that brings a network interface up might not be very useful? So we might not be able to find it in public repositories.
Re: [PATCH net-next RFC v4] net: hdlc_x25: Queue outgoing LAPB frames
On Fri, Feb 19, 2021 at 10:39 AM Jakub Kicinski wrote: > > Not entirely sure what the argument is about but adding constants would > certainly help. Leon wants me to replace this: dev->needed_headroom = 3 - 1; with this: /* 2 is the result of 3 - 1 */ dev->needed_headroom = 2; But I don't feel his way is better than my way. > More fundamentally IDK if we can make such a fundamental change here. > When users upgrade from older kernel are all their scripts going to > work the same? Won't they have to bring the new netdev up? Yes, this patch will break backward compatibility. Users with old scripts will find them no longer working. However, it's hard for me to find a better way to solve the problem described in the commit message. So I sent this as an RFC to see what people think about this. (Martin Schiller seems to be OK with this.) I think users who don't use scripts can adapt quickly and users who use scripts can also trivally fix their scripts. Actually many existing commits in the kernel also (more or less) cause some user-visible changes. But I admit this patch is a really big change.
Re: [PATCH net-next RFC v4] net: hdlc_x25: Queue outgoing LAPB frames
On Thu, Feb 18, 2021 at 12:06 PM Xie He wrote: > > On Thu, Feb 18, 2021 at 11:55 AM Leon Romanovsky wrote: > > > > This is how we write code, we use defines instead of constant numbers, > > comments to describe tricky parts and assign already preprocessed result. > > > > There is nothing I can do If you don't like or don't want to use Linux > > kernel > > style. > > So what is your suggestion exactly? Use defines or write comments? > > As I understand, you want to replace the "3 - 1" with "2", and then > write comments to explain that this "2" is the result of "3 - 1". > > Why do you want to do this? You are doing useless things and you force > readers of this code to think about useless things. > > You said this was "Linux kernel style"? Why? Which sentence of the > Linux kernel style guide suggests your way is better than my way? Nevermind, if you *really* want me to replace this "3 - 1" with "2" and explain in the comment that the "2" is a result of "3 - 1". I'll do this. I admit this is a style issue. So it is hard to argue and reach an agreement. Just reply with a request and I'll make the change. However I'm not able to agree with you in my heart. On Thu, Feb 18, 2021 at 12:06 PM Xie He wrote: > > On Thu, Feb 18, 2021 at 11:55 AM Leon Romanovsky wrote: > > > > This is how we write code, we use defines instead of constant numbers, > > comments to describe tricky parts and assign already preprocessed result. > > > > There is nothing I can do If you don't like or don't want to use Linux > > kernel > > style. > > So what is your suggestion exactly? Use defines or write comments? > > As I understand, you want to replace the "3 - 1" with "2", and then > write comments to explain that this "2" is the result of "3 - 1". > > Why do you want to do this? You are doing useless things and you force > readers of this code to think about useless things. > > You said this was "Linux kernel style"? Why? Which sentence of the > Linux kernel style guide suggests your way is better than my way?
Re: [PATCH net-next RFC v4] net: hdlc_x25: Queue outgoing LAPB frames
On Thu, Feb 18, 2021 at 11:55 AM Leon Romanovsky wrote: > > This is how we write code, we use defines instead of constant numbers, > comments to describe tricky parts and assign already preprocessed result. > > There is nothing I can do If you don't like or don't want to use Linux kernel > style. So what is your suggestion exactly? Use defines or write comments? As I understand, you want to replace the "3 - 1" with "2", and then write comments to explain that this "2" is the result of "3 - 1". Why do you want to do this? You are doing useless things and you force readers of this code to think about useless things. You said this was "Linux kernel style"? Why? Which sentence of the Linux kernel style guide suggests your way is better than my way?
Re: [PATCH net-next RFC v4] net: hdlc_x25: Queue outgoing LAPB frames
On Thu, Feb 18, 2021 at 2:37 AM Leon Romanovsky wrote: > > It is not me who didn't explain, it is you who didn't want to write clear > comment that describes the headroom size without need of "3 - 1". Why do I need to write unnecessary comments when "3 - 1" and the current comment already explains everything? > So in current situation, you added two things: comment and assignment. > Both of them aren't serve their goals. Why? > Your comment doesn't explain > enough and needs extra help Why? My comment already explains everything. > and your assignment is useless without > comment. My assignment is already very clear with my current comment. My comment explains very clearly what this assignment means.
Re: [PATCH net-next RFC v4] net: hdlc_x25: Queue outgoing LAPB frames
On Thu, Feb 18, 2021 at 12:57 AM Leon Romanovsky wrote: > > It is nice that you are resending your patch without the resolution. > However it will be awesome if you don't ignore review comments and fix this > "3 - 1" > by writing solid comment above. I thought you already agreed with me? It looks like you didn't? I still don't think there is any problem with my current way. I still don't understand your point. What problem do you think is there? Why is your way better than my way? I've already given multiple reasons about why my way is better than yours. But you didn't explain clearly why yours is better than mine.
[PATCH net-next RFC v4] net: hdlc_x25: Queue outgoing LAPB frames
When sending packets, we will first hand over the (L3) packets to the
LAPB module. The LAPB module will then hand over the corresponding LAPB
(L2) frames back to us for us to transmit.
The LAPB module can also emit LAPB (L2) frames at any time, even without
an (L3) packet currently being sent on the device. This happens when the
LAPB module tries to send (L3) packets queued up in its internal queue,
or when the LAPB module decides to send some (L2) control frame.
This means we need to have a queue for these outgoing LAPB (L2) frames,
otherwise frames can be dropped if sent when the hardware driver is
already busy in transmitting. The queue needs to be controlled by
the hardware driver's netif_stop_queue and netif_wake_queue calls.
Therefore, we need to use the device's qdisc TX queue for this purpose.
However, currently outgoing LAPB (L2) frames are not queued.
On the other hand, outgoing (L3) packets (before they are handed over
to the LAPB module) don't need to be queued, because the LAPB module
already has an internal queue for them, and is able to queue new outgoing
(L3) packets at any time. However, currently outgoing (L3) packets are
being queued in the device's qdisc TX queue, which is controlled by
the hardware driver's netif_stop_queue and netif_wake_queue calls.
This is unnecessary and meaningless.
To fix these issues, we can split the HDLC device into two devices -
a virtual X.25 device and the actual HDLC device, use the virtual X.25
device to send (L3) packets and then use the actual HDLC device to
queue LAPB (L2) frames. The outgoing (L2) LAPB queue will be controlled
by the hardware driver's netif_stop_queue and netif_wake_queue calls,
while outgoing (L3) packets will not be affected by these calls.
Cc: Martin Schiller
Signed-off-by: Xie He
---
Change from RFC v3:
Call netif_carrier_off in x25_hdlc_open before calling register_netdevice.
Change from RFC v2:
Simplified the commit message.
Dropped the x25_open fix which is already merged into net-next now.
Use HDLC_MAX_MTU as the mtu of the X.25 virtual device.
Add an explanation to the documentation about the X.25 virtual device.
Change from RFC v1:
Properly initialize state(hdlc)->x25_dev and state(hdlc)->x25_dev_lock.
---
Documentation/networking/generic-hdlc.rst | 3 +
drivers/net/wan/hdlc_x25.c| 156 ++
2 files changed, 133 insertions(+), 26 deletions(-)
diff --git a/Documentation/networking/generic-hdlc.rst
b/Documentation/networking/generic-hdlc.rst
index 1c3bb5cb98d4..55f6b0ab45be 100644
--- a/Documentation/networking/generic-hdlc.rst
+++ b/Documentation/networking/generic-hdlc.rst
@@ -59,6 +59,9 @@ or::
In Frame Relay mode, ifconfig master hdlc device up (without assigning
any IP address to it) before using pvc devices.
+In X.25 mode, ifconfig the hdlc device up, then a virtual X.25 device
+would appear for use.
+
Setting interface:
diff --git a/drivers/net/wan/hdlc_x25.c b/drivers/net/wan/hdlc_x25.c
index 4aaa6388b9ee..b7744065900f 100644
--- a/drivers/net/wan/hdlc_x25.c
+++ b/drivers/net/wan/hdlc_x25.c
@@ -23,6 +23,13 @@
struct x25_state {
x25_hdlc_proto settings;
+ struct net_device *x25_dev;
+ spinlock_t x25_dev_lock; /* Protects the x25_dev pointer */
+};
+
+/* Pointed to by netdev_priv(x25_dev) */
+struct x25_device {
+ struct net_device *hdlc_dev;
};
static int x25_ioctl(struct net_device *dev, struct ifreq *ifr);
@@ -32,6 +39,11 @@ static struct x25_state *state(hdlc_device *hdlc)
return hdlc->state;
}
+static struct x25_device *dev_to_x25(struct net_device *dev)
+{
+ return netdev_priv(dev);
+}
+
/* These functions are callbacks called by LAPB layer */
static void x25_connect_disconnect(struct net_device *dev, int reason, int
code)
@@ -89,15 +101,10 @@ static int x25_data_indication(struct net_device *dev,
struct sk_buff *skb)
static void x25_data_transmit(struct net_device *dev, struct sk_buff *skb)
{
- hdlc_device *hdlc = dev_to_hdlc(dev);
-
+ skb->dev = dev_to_x25(dev)->hdlc_dev;
+ skb->protocol = htons(ETH_P_HDLC);
skb_reset_network_header(skb);
- skb->protocol = hdlc_type_trans(skb, dev);
-
- if (dev_nit_active(dev))
- dev_queue_xmit_nit(skb, dev);
-
- hdlc->xmit(skb, dev); /* Ignore return value :-( */
+ dev_queue_xmit(skb);
}
@@ -163,7 +170,8 @@ static int x25_open(struct net_device *dev)
.data_indication = x25_data_indication,
.data_transmit = x25_data_transmit,
};
- hdlc_device *hdlc = dev_to_hdlc(dev);
+ struct net_device *hdlc_dev = dev_to_x25(dev)->hdlc_dev;
+ hdlc_device *hdlc = dev_to_hdlc(hdlc_dev);
struct lapb_parms_struct params;
int result;
@@ -195,9 +203,101 @@ static int x25_open(struct net_device *dev)
-static void x25_close(struct net_device *dev)
+static int x25_close(struct net_device *dev)
{
Re: [PATCH net-next RFC v3] net: hdlc_x25: Queue outgoing LAPB frames
On Mon, Feb 15, 2021 at 10:04 PM Leon Romanovsky wrote: > > On Mon, Feb 15, 2021 at 11:08:02AM -0800, Xie He wrote: > > On Mon, Feb 15, 2021 at 10:54 AM Leon Romanovsky wrote: > > > > > > On Mon, Feb 15, 2021 at 09:23:32AM -0800, Xie He wrote: > > > > On Mon, Feb 15, 2021 at 1:25 AM Leon Romanovsky wrote: > > > > > > > > > > > + /* When transmitting data: > > > > > > + * first we'll remove a pseudo header of 1 byte, > > > > > > + * then the LAPB module will prepend an LAPB header of at > > > > > > most 3 bytes. > > > > > > + */ > > > > > > + dev->needed_headroom = 3 - 1; > > > > > > > > > > 3 - 1 = 2 > > > > > > > > > > Thanks > > > > > > > > Actually this is intentional. It makes the numbers more meaningful. > > > > > > > > The compiler should automatically generate the "2" so there would be > > > > no runtime penalty. > > > > > > If you want it intentional, write it in the comment. > > > > > > /* When transmitting data, we will need extra 2 bytes headroom, > > > * which are 3 bytes of LAPB header minus one byte of pseudo header. > > > */ > > > dev->needed_headroom = 2; > > > > I think this is unnecessary. The current comment already explains the > > meaning of the "1" and the "3". There's no need for a reader of this > > code to understand what a "2" is. That is the job of the compiler, not > > the human reader. > > It is not related to compiler/human format. If you need to write "3 - 1" > to make it easy for users, it means that your comment above is not > full/correct/e.t.c. My point is: there is no need for human programmers / code readers to understand what this "2" is. There is no need to explain what this "2" means in the comment. There is no need to write this "2" in the code. There is no need for this "2" to appear anywhere. That is just an intermediate result generated by the compiler. It is similar to assembly or machine code. It is generated by the compiler. Human programmers just don't care about this intermediate result. My point could be more apparent if you consider a more complex situation: "3 - 1 + 2 + 4 + 2". No human would want to see a meaningless "10" in the code. We want to see the meaning of the numbers, not a meaningless intermediate calculation result.
Re: [PATCH net-next RFC v3] net: hdlc_x25: Queue outgoing LAPB frames
On Mon, Feb 15, 2021 at 10:54 AM Leon Romanovsky wrote: > > On Mon, Feb 15, 2021 at 09:23:32AM -0800, Xie He wrote: > > On Mon, Feb 15, 2021 at 1:25 AM Leon Romanovsky wrote: > > > > > > > + /* When transmitting data: > > > > + * first we'll remove a pseudo header of 1 byte, > > > > + * then the LAPB module will prepend an LAPB header of at most 3 > > > > bytes. > > > > + */ > > > > + dev->needed_headroom = 3 - 1; > > > > > > 3 - 1 = 2 > > > > > > Thanks > > > > Actually this is intentional. It makes the numbers more meaningful. > > > > The compiler should automatically generate the "2" so there would be > > no runtime penalty. > > If you want it intentional, write it in the comment. > > /* When transmitting data, we will need extra 2 bytes headroom, > * which are 3 bytes of LAPB header minus one byte of pseudo header. > */ > dev->needed_headroom = 2; I think this is unnecessary. The current comment already explains the meaning of the "1" and the "3". There's no need for a reader of this code to understand what a "2" is. That is the job of the compiler, not the human reader.
Re: [PATCH net-next RFC v3] net: hdlc_x25: Queue outgoing LAPB frames
On Mon, Feb 15, 2021 at 1:25 AM Leon Romanovsky wrote: > > > + /* When transmitting data: > > + * first we'll remove a pseudo header of 1 byte, > > + * then the LAPB module will prepend an LAPB header of at most 3 > > bytes. > > + */ > > + dev->needed_headroom = 3 - 1; > > 3 - 1 = 2 > > Thanks Actually this is intentional. It makes the numbers more meaningful. The compiler should automatically generate the "2" so there would be no runtime penalty.
[PATCH net-next RFC v3] net: hdlc_x25: Queue outgoing LAPB frames
When sending packets, we will first hand over the (L3) packets to the
LAPB module. The LAPB module will then hand over the corresponding LAPB
(L2) frames back to us for us to transmit.
The LAPB module can also emit LAPB (L2) frames at any time, even without
an (L3) packet currently being sent on the device. This happens when the
LAPB module tries to send (L3) packets queued up in its internal queue,
or when the LAPB module decides to send some (L2) control frame.
This means we need to have a queue for these outgoing LAPB (L2) frames,
otherwise frames can be dropped if sent when the hardware driver is
already busy in transmitting. The queue needs to be controlled by
the hardware driver's netif_stop_queue and netif_wake_queue calls.
Therefore, we need to use the device's qdisc TX queue for this purpose.
However, currently outgoing LAPB (L2) frames are not queued.
On the other hand, outgoing (L3) packets (before they are handed over
to the LAPB module) don't need to be queued, because the LAPB module
already has an internal queue for them, and is able to queue new outgoing
(L3) packets at any time. However, currently outgoing (L3) packets are
being queued in the device's qdisc TX queue, which is controlled by
the hardware driver's netif_stop_queue and netif_wake_queue calls.
This is unnecessary and meaningless.
To fix these issues, we can split the HDLC device into two devices -
a virtual X.25 device and the actual HDLC device, use the virtual X.25
device to send (L3) packets and then use the actual HDLC device to
queue LAPB (L2) frames. The outgoing (L2) LAPB queue will be controlled
by the hardware driver's netif_stop_queue and netif_wake_queue calls,
while outgoing (L3) packets will not be affected by these calls.
Cc: Martin Schiller
Signed-off-by: Xie He
---
Change from RFC v2:
Simplified the commit message.
Dropped the x25_open fix which is already merged into net-next now.
Use HDLC_MAX_MTU as the mtu of the X.25 virtual device.
Add an explanation to the documentation about the X.25 virtual device.
Change from RFC v1:
Properly initialize state(hdlc)->x25_dev and state(hdlc)->x25_dev_lock.
---
Documentation/networking/generic-hdlc.rst | 3 +
drivers/net/wan/hdlc_x25.c| 153 ++
2 files changed, 130 insertions(+), 26 deletions(-)
diff --git a/Documentation/networking/generic-hdlc.rst
b/Documentation/networking/generic-hdlc.rst
index 1c3bb5cb98d4..55f6b0ab45be 100644
--- a/Documentation/networking/generic-hdlc.rst
+++ b/Documentation/networking/generic-hdlc.rst
@@ -59,6 +59,9 @@ or::
In Frame Relay mode, ifconfig master hdlc device up (without assigning
any IP address to it) before using pvc devices.
+In X.25 mode, ifconfig the hdlc device up, then a virtual X.25 device
+would appear for use.
+
Setting interface:
diff --git a/drivers/net/wan/hdlc_x25.c b/drivers/net/wan/hdlc_x25.c
index 4aaa6388b9ee..b0541f1bfb4e 100644
--- a/drivers/net/wan/hdlc_x25.c
+++ b/drivers/net/wan/hdlc_x25.c
@@ -23,6 +23,13 @@
struct x25_state {
x25_hdlc_proto settings;
+ struct net_device *x25_dev;
+ spinlock_t x25_dev_lock; /* Protects the x25_dev pointer */
+};
+
+/* Pointed to by netdev_priv(x25_dev) */
+struct x25_device {
+ struct net_device *hdlc_dev;
};
static int x25_ioctl(struct net_device *dev, struct ifreq *ifr);
@@ -32,6 +39,11 @@ static struct x25_state *state(hdlc_device *hdlc)
return hdlc->state;
}
+static struct x25_device *dev_to_x25(struct net_device *dev)
+{
+ return netdev_priv(dev);
+}
+
/* These functions are callbacks called by LAPB layer */
static void x25_connect_disconnect(struct net_device *dev, int reason, int
code)
@@ -89,15 +101,10 @@ static int x25_data_indication(struct net_device *dev,
struct sk_buff *skb)
static void x25_data_transmit(struct net_device *dev, struct sk_buff *skb)
{
- hdlc_device *hdlc = dev_to_hdlc(dev);
-
+ skb->dev = dev_to_x25(dev)->hdlc_dev;
+ skb->protocol = htons(ETH_P_HDLC);
skb_reset_network_header(skb);
- skb->protocol = hdlc_type_trans(skb, dev);
-
- if (dev_nit_active(dev))
- dev_queue_xmit_nit(skb, dev);
-
- hdlc->xmit(skb, dev); /* Ignore return value :-( */
+ dev_queue_xmit(skb);
}
@@ -163,7 +170,8 @@ static int x25_open(struct net_device *dev)
.data_indication = x25_data_indication,
.data_transmit = x25_data_transmit,
};
- hdlc_device *hdlc = dev_to_hdlc(dev);
+ struct net_device *hdlc_dev = dev_to_x25(dev)->hdlc_dev;
+ hdlc_device *hdlc = dev_to_hdlc(hdlc_dev);
struct lapb_parms_struct params;
int result;
@@ -195,9 +203,98 @@ static int x25_open(struct net_device *dev)
-static void x25_close(struct net_device *dev)
+static int x25_close(struct net_device *dev)
{
lapb_unregister(dev);
+ return 0;
+}
+
+static const struct net_device_ops hdlc_x25_netde
Re: [PATCH net-next RFC v2] net: hdlc_x25: Queue outgoing LAPB frames
On Sun, Feb 14, 2021 at 10:27 PM Martin Schiller wrote: > > At first glance, the patch looks quite reasonable. The only thing I > noticed right away is that you also included the changes of your patch > "Return meaningful error code in x25_open". Thanks! It was because this patch was sent before that fix got merged into "net-next". I will drop that part now. I will also make the MTU of the virtual X.25 device be HDLC_MAX_MTU (instead of HDLC_MAX_MTU - 2), because I see other HDLC Protocol Drivers seem to also use this value as MTU (without subtracting the header length). > I hope to get back to the office this week and test it. Thanks!
[PATCH net-next RFC v2] net: hdlc_x25: Queue outgoing LAPB frames
When sending packets, we will first hand over the (L3) packets to the
LAPB module, then the LAPB module will hand over the corresponding LAPB
(L2) frames back to us for us to transmit.
The LAPB module can also emit LAPB (L2) frames at any time without an
(L3) packet currently being sent, when it is trying to send (L3) packets
queued up in its internal queue, or when it decides to send some (L2)
control frame.
This means we need have a queue for these outgoing LAPB (L2) frames to
avoid congestion. This queue needs to be controlled by the hardware
drivers' netif_stop_queue and netif_wake_queue calls. So we need to use
a qdisc TX queue for this purpose.
On the other hand, the outgoing (L3) packets don't need to be queued,
because the LAPB module already has an internal queue for them.
However, currently the outgoing LAPB (L2) frames are not queued. This
can cause frames to be dropped by hardware drivers when they are busy.
At the same time the (L3) packets are being queued and controlled by
hardware drivers' netif_stop_queue and netif_wake_queue calls. This is
unnecessary and meaningless.
To solve this problem, we can split the HDLC device into two devices:
a virtual X.25 device and an actual HDLC device, using the virtual
X.25 device to send (L3) packets and using the actual HDLC device to
queue LAPB (L2) frames. The outgoing LAPB queue will be controlled by
hardware drivers' netif_stop_queue and netif_wake_queue calls, while
outgoing (L3) packets will not be affected by these calls.
Cc: Martin Schiller
Signed-off-by: Xie He
---
Change from RFC v1:
Properly initialize state(hdlc)->x25_dev and state(hdlc)->x25_dev_lock.
---
drivers/net/wan/hdlc_x25.c | 158 ++---
1 file changed, 129 insertions(+), 29 deletions(-)
diff --git a/drivers/net/wan/hdlc_x25.c b/drivers/net/wan/hdlc_x25.c
index bb164805804e..2a7b3c3d0c05 100644
--- a/drivers/net/wan/hdlc_x25.c
+++ b/drivers/net/wan/hdlc_x25.c
@@ -23,6 +23,13 @@
struct x25_state {
x25_hdlc_proto settings;
+ struct net_device *x25_dev;
+ spinlock_t x25_dev_lock; /* Protects the x25_dev pointer */
+};
+
+/* Pointed to by netdev_priv(x25_dev) */
+struct x25_device {
+ struct net_device *hdlc_dev;
};
static int x25_ioctl(struct net_device *dev, struct ifreq *ifr);
@@ -32,6 +39,11 @@ static struct x25_state *state(hdlc_device *hdlc)
return hdlc->state;
}
+static struct x25_device *dev_to_x25(struct net_device *dev)
+{
+ return netdev_priv(dev);
+}
+
/* These functions are callbacks called by LAPB layer */
static void x25_connect_disconnect(struct net_device *dev, int reason, int
code)
@@ -89,15 +101,10 @@ static int x25_data_indication(struct net_device *dev,
struct sk_buff *skb)
static void x25_data_transmit(struct net_device *dev, struct sk_buff *skb)
{
- hdlc_device *hdlc = dev_to_hdlc(dev);
-
+ skb->dev = dev_to_x25(dev)->hdlc_dev;
+ skb->protocol = htons(ETH_P_HDLC);
skb_reset_network_header(skb);
- skb->protocol = hdlc_type_trans(skb, dev);
-
- if (dev_nit_active(dev))
- dev_queue_xmit_nit(skb, dev);
-
- hdlc->xmit(skb, dev); /* Ignore return value :-( */
+ dev_queue_xmit(skb);
}
@@ -163,17 +170,18 @@ static int x25_open(struct net_device *dev)
.data_indication = x25_data_indication,
.data_transmit = x25_data_transmit,
};
- hdlc_device *hdlc = dev_to_hdlc(dev);
+ struct net_device *hdlc_dev = dev_to_x25(dev)->hdlc_dev;
+ hdlc_device *hdlc = dev_to_hdlc(hdlc_dev);
struct lapb_parms_struct params;
int result;
result = lapb_register(dev, );
if (result != LAPB_OK)
- return result;
+ return -ENOMEM;
result = lapb_getparms(dev, );
if (result != LAPB_OK)
- return result;
+ return -EINVAL;
if (state(hdlc)->settings.dce)
params.mode = params.mode | LAPB_DCE;
@@ -188,16 +196,104 @@ static int x25_open(struct net_device *dev)
result = lapb_setparms(dev, );
if (result != LAPB_OK)
- return result;
+ return -EINVAL;
return 0;
}
-static void x25_close(struct net_device *dev)
+static int x25_close(struct net_device *dev)
{
lapb_unregister(dev);
+ return 0;
+}
+
+static const struct net_device_ops hdlc_x25_netdev_ops = {
+ .ndo_open = x25_open,
+ .ndo_stop = x25_close,
+ .ndo_start_xmit = x25_xmit,
+};
+
+static void x25_setup_virtual_dev(struct net_device *dev)
+{
+ dev->netdev_ops = _x25_netdev_ops;
+ dev->type= ARPHRD_X25;
+ dev->addr_len= 0;
+ dev->hard_header_len = 0;
+}
+
+static int x25_hdlc_open(struct net_device *dev)
+{
+ struct hdlc_device *hdlc = dev_to_hdlc(dev);
+ struct net_device *x25_dev;
+
[PATCH net-next RFC] net: hdlc_x25: Queue outgoing LAPB frames
When sending packets, we will first hand over the (L3) packets to the
LAPB module, then the LAPB module will hand over the corresponding LAPB
(L2) frames back to us for us to transmit.
The LAPB module can also emit LAPB (L2) frames at any time without an
(L3) packet currently being sent, when it is trying to send (L3) packets
queued up in its internal queue, or when it decides to send some (L2)
control frame.
This means we need have a queue for these outgoing LAPB (L2) frames to
avoid congestion. This queue needs to be controlled by the hardware
drivers' netif_stop_queue and netif_wake_queue calls. So we need to use
a qdisc TX queue for this purpose.
On the other hand, the outgoing (L3) packets don't need to be queued,
because the LAPB module already has an internal queue for them.
However, currently the outgoing LAPB (L2) frames are not queued. This
can cause frames to be dropped by hardware drivers when they are busy.
At the same time the (L3) packets are being queued and controlled by
hardware drivers' netif_stop_queue and netif_wake_queue calls. This is
unnecessary and meaningless.
To solve this problem, we can split the HDLC device into two devices:
a virtual X.25 device and an actual HDLC device, using the virtual
X.25 device to send (L3) packets and using the actual HDLC device to
queue LAPB (L2) frames. The outgoing LAPB queue will be controlled by
hardware drivers' netif_stop_queue and netif_wake_queue calls, while
outgoing (L3) packets will not be affected by these calls.
Cc: Martin Schiller
Signed-off-by: Xie He
---
drivers/net/wan/hdlc_x25.c | 156 ++---
1 file changed, 127 insertions(+), 29 deletions(-)
diff --git a/drivers/net/wan/hdlc_x25.c b/drivers/net/wan/hdlc_x25.c
index bb164805804e..f21aaee364e2 100644
--- a/drivers/net/wan/hdlc_x25.c
+++ b/drivers/net/wan/hdlc_x25.c
@@ -23,6 +23,13 @@
struct x25_state {
x25_hdlc_proto settings;
+ struct net_device *x25_dev;
+ spinlock_t x25_dev_lock; /* Protects the x25_dev pointer */
+};
+
+/* Pointed to by netdev_priv(x25_dev) */
+struct x25_device {
+ struct net_device *hdlc_dev;
};
static int x25_ioctl(struct net_device *dev, struct ifreq *ifr);
@@ -32,6 +39,11 @@ static struct x25_state *state(hdlc_device *hdlc)
return hdlc->state;
}
+static struct x25_device *dev_to_x25(struct net_device *dev)
+{
+ return netdev_priv(dev);
+}
+
/* These functions are callbacks called by LAPB layer */
static void x25_connect_disconnect(struct net_device *dev, int reason, int
code)
@@ -89,15 +101,10 @@ static int x25_data_indication(struct net_device *dev,
struct sk_buff *skb)
static void x25_data_transmit(struct net_device *dev, struct sk_buff *skb)
{
- hdlc_device *hdlc = dev_to_hdlc(dev);
-
+ skb->dev = dev_to_x25(dev)->hdlc_dev;
+ skb->protocol = htons(ETH_P_HDLC);
skb_reset_network_header(skb);
- skb->protocol = hdlc_type_trans(skb, dev);
-
- if (dev_nit_active(dev))
- dev_queue_xmit_nit(skb, dev);
-
- hdlc->xmit(skb, dev); /* Ignore return value :-( */
+ dev_queue_xmit(skb);
}
@@ -163,17 +170,18 @@ static int x25_open(struct net_device *dev)
.data_indication = x25_data_indication,
.data_transmit = x25_data_transmit,
};
- hdlc_device *hdlc = dev_to_hdlc(dev);
+ struct net_device *hdlc_dev = dev_to_x25(dev)->hdlc_dev;
+ hdlc_device *hdlc = dev_to_hdlc(hdlc_dev);
struct lapb_parms_struct params;
int result;
result = lapb_register(dev, );
if (result != LAPB_OK)
- return result;
+ return -ENOMEM;
result = lapb_getparms(dev, );
if (result != LAPB_OK)
- return result;
+ return -EINVAL;
if (state(hdlc)->settings.dce)
params.mode = params.mode | LAPB_DCE;
@@ -188,16 +196,104 @@ static int x25_open(struct net_device *dev)
result = lapb_setparms(dev, );
if (result != LAPB_OK)
- return result;
+ return -EINVAL;
return 0;
}
-static void x25_close(struct net_device *dev)
+static int x25_close(struct net_device *dev)
{
lapb_unregister(dev);
+ return 0;
+}
+
+static const struct net_device_ops hdlc_x25_netdev_ops = {
+ .ndo_open = x25_open,
+ .ndo_stop = x25_close,
+ .ndo_start_xmit = x25_xmit,
+};
+
+static void x25_setup_virtual_dev(struct net_device *dev)
+{
+ dev->netdev_ops = _x25_netdev_ops;
+ dev->type= ARPHRD_X25;
+ dev->addr_len= 0;
+ dev->hard_header_len = 0;
+}
+
+static int x25_hdlc_open(struct net_device *dev)
+{
+ struct hdlc_device *hdlc = dev_to_hdlc(dev);
+ struct net_device *x25_dev;
+ char x25_dev_name[sizeof(x25_dev->name)];
+ int result;
+
+ if (strle
Re: [PATCH net-next] net/packet: Improve the comment about LL header visibility criteria
On Sat, Feb 6, 2021 at 7:43 AM Eyal Birger wrote: > > As such, may I suggest making this explicit by making > dev_hard_header() use dev_has_header()? That is a good idea. We may submit another patch for that.
[PATCH net-next] net/packet: Improve the comment about LL header visibility criteria
The "dev_has_header" function, recently added in
commit d549699048b4 ("net/packet: fix packet receive on L3 devices
without visible hard header"),
is more accurate as criteria for determining whether a device exposes
the LL header to upper layers, because in addition to dev->header_ops,
it also checks for dev->header_ops->create.
When transmitting an skb on a device, dev_hard_header can be called to
generate an LL header. dev_hard_header will only generate a header if
dev->header_ops->create is present.
Signed-off-by: Xie He
---
net/packet/af_packet.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 6bbc7a448593..e24b2841c643 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -132,17 +132,17 @@ Resume
because it is invisible to us.
On transmit:
-dev->header_ops != NULL
+dev_has_header(dev) == true
mac_header -> ll header
data -> ll header
-dev->header_ops == NULL (ll header is invisible to us)
+dev_has_header(dev) == false (ll header is invisible to us)
mac_header -> data
data -> data
We should set network_header on output to the correct position,
packet classifier depends on it.
*/
--
2.27.0
[PATCH net] net: hdlc_x25: Return meaningful error code in x25_open
It's not meaningful to pass on LAPB error codes to HDLC code or other
parts of the system, because they will not understand the error codes.
Instead, use system-wide recognizable error codes.
Fixes: f362e5fe0f1f ("wan/hdlc_x25: make lapb params configurable")
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: Martin Schiller
Signed-off-by: Xie He
---
drivers/net/wan/hdlc_x25.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/net/wan/hdlc_x25.c b/drivers/net/wan/hdlc_x25.c
index bb164805804e..4aaa6388b9ee 100644
--- a/drivers/net/wan/hdlc_x25.c
+++ b/drivers/net/wan/hdlc_x25.c
@@ -169,11 +169,11 @@ static int x25_open(struct net_device *dev)
result = lapb_register(dev, );
if (result != LAPB_OK)
- return result;
+ return -ENOMEM;
result = lapb_getparms(dev, );
if (result != LAPB_OK)
- return result;
+ return -EINVAL;
if (state(hdlc)->settings.dce)
params.mode = params.mode | LAPB_DCE;
@@ -188,7 +188,7 @@ static int x25_open(struct net_device *dev)
result = lapb_setparms(dev, );
if (result != LAPB_OK)
- return result;
+ return -EINVAL;
return 0;
}
--
2.27.0
Re: [PATCH net] net: lapb: Copy the skb before sending a packet
On Mon, Feb 1, 2021 at 8:42 PM Jakub Kicinski wrote: > > On Mon, 1 Feb 2021 08:14:31 -0800 Xie He wrote: > > On Mon, Feb 1, 2021 at 6:10 AM Julian Wiedmann wrote: > > > This sounds a bit like you want skb_cow_head() ... ? > > > > Calling "skb_cow_head" before we call "skb_clone" would indeed solve > > the problem of writes to our clones affecting clones in other parts of > > the system. But since we are still writing to the skb after > > "skb_clone", it'd still be better to replace "skb_clone" with > > "skb_copy" to avoid interference between our own clones. > > Why call skb_cow_head() before skb_clone()? skb_cow_head should be > called before the data in skb head is modified. I'm assuming you're only > modifying "front" of the frame, right? skb_cow_head() should do nicely > in that case. The modification happens after skb_clone. If we call skb_cow_head after skb_clone (before the modification), then skb_cow_head would always see that the skb is a clone and would always copy it. Therefore skb_clone + skb_cow_head is equivalent to skb_copy.
Re: [PATCH net] net: lapb: Copy the skb before sending a packet
On Mon, Feb 1, 2021 at 6:10 AM Julian Wiedmann wrote: > > This sounds a bit like you want skb_cow_head() ... ? Calling "skb_cow_head" before we call "skb_clone" would indeed solve the problem of writes to our clones affecting clones in other parts of the system. But since we are still writing to the skb after "skb_clone", it'd still be better to replace "skb_clone" with "skb_copy" to avoid interference between our own clones.
Re: [PATCH net] net: hdlc_x25: Use qdisc to queue outgoing LAPB frames
On Mon, Feb 1, 2021 at 5:14 AM Martin Schiller wrote: > > But control frames are currently sent past the lapb write_queue. > So another queue would have to be created. > > And wouldn't it be better to have it in the hdlc_x25 driver, leaving > LAPB unaffected? Hmm.. Indeed. I agree. I also think the queue needs to be the qdisc queue, so that it'll be able to respond immediately to hardware drivers' netif_wake_queue call. Initially I was considering using the qdisc of the HDLC device to queue the outgoing L2 frames again (after their corresponding L3 packets having already gone through the queue). But Jakub didn't like the idea of queuing the same data twice. I also found that if an L3 packet was sent through the qdisc without being queued, and LAPB didn't queue it either, then the emitted L2 frame must be queued in the qdisc. This is both not optimal and causing problems when using the "noqueue" qdisc. Maybe the only way is to create a virtual device on top of the HDLC device, using the virtual device to queue L3 packets and using the actual HDLC device to queue L2 frames.
Re: [PATCH net] net: hdlc_x25: Use qdisc to queue outgoing LAPB frames
On Mon, Feb 1, 2021 at 1:18 AM Martin Schiller wrote: > > I have thought about this issue again. > > I also have to say that I have never noticed any problems in this area > before. > > So again for (my) understanding: > When a hardware driver calls netif_stop_queue, the frames sent from > layer 3 (X.25) with dev_queue_xmit are queued and not passed "directly" > to x25_xmit of the hdlc_x25 driver. > > So nothing is added to the write_queue anymore (except possibly > un-acked-frames by lapb_requeue_frames). If the LAPB module only emits an L2 frame when an L3 packet comes from the upper layer, then yes, there would be no problem because the L3 packet is already controlled by the qdisc and there is no need to control the corresponding L2 frame again. However, the LAPB module can emits L2 frames when there's no L3 packet coming, when 1) there are some packets queued in the LAPB module's internal queue; and 2) the LAPB decides to send some control frame (e.g. by the timers). > Shouldn't it actually be sufficient to check for netif_queue_stopped in > lapb_kick and then do "nothing" if necessary? We can consider this situation: When the upper layer has nothing to send, but there are some packets in the LAPB module's internal queue waiting to be sent. The LAPB module will try to send the packets, but after it has sent out the first packet, it will meet the "queue stopped" situation. In this situation, it'd be preferable to immediately start sending the second packet after the queue is started again. "Doing nothing" in this situation would mean waiting until some other events occur, such as receiving responses from the other side, or receiving more outgoing packets from L3. > As soon as the hardware driver calls netif_wake_queue, the whole thing > should just continue running. This relies on the fact that the upper layer has something to send. If the upper layer has nothing to send, lapb_kick would not be automatically called again until some other events occur (such as receiving responses from the other side). I think it'd be better if we do not rely on the assumption that L3 is going to send more packets to us, as L3 itself would assume us to provide it a reliable link service and we should fulfill its expectation.
Re: [PATCH net] net: lapb: Copy the skb before sending a packet
On Mon, Feb 1, 2021 at 2:05 AM Martin Schiller wrote: > > What kind of packages do you mean are corrupted? > ETH_P_X25 or ETH_P_HDLC? I mean ETH_P_X25. I was using "lapbether.c" to test so there was no ETH_P_HDLC. > I have also sent a patch here in the past that addressed corrupted > ETH_P_X25 frames on an AF_PACKET socket: > > https://lkml.org/lkml/2020/1/13/388 > > Unfortunately I could not track and describe exactly where the problem > was. Ah... Looks like we had the same problem. > I just wonder when/where is the logically correct place to copy the skb. > Shouldn't it be copied before removing the pseudo header (as I did in my > patch)? I think it's not necessary to copy it before removing the pseudo header, because "skb_pull" will not change any data in the data buffer. "skb_pull" will only change the values of "skb->data" and "skb->len". These values are not shared between clones of skbs, so it's safe to change them without affecting other clones of the skb. I also choose to copy the skb in the LAPB module (rather than in the drivers) because I see all drivers have this problem (including the recently deleted x25_asy.c driver), so it'd be better to fix this issue in the LAPB module, for all drivers.
[PATCH net] net: lapb: Copy the skb before sending a packet
When sending a packet, we will prepend it with an LAPB header.
This modifies the shared parts of a cloned skb, so we should copy the
skb rather than just clone it, before we prepend the header.
In "Documentation/networking/driver.rst" (the 2nd point), it states
that drivers shouldn't modify the shared parts of a cloned skb when
transmitting.
The "dev_queue_xmit_nit" function in "net/core/dev.c", which is called
when an skb is being sent, clones the skb and sents the clone to
AF_PACKET sockets. Because the LAPB drivers first remove a 1-byte
pseudo-header before handing over the skb to us, if we don't copy the
skb before prepending the LAPB header, the first byte of the packets
received on AF_PACKET sockets can be corrupted.
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: Martin Schiller
Signed-off-by: Xie He
---
net/lapb/lapb_out.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/net/lapb/lapb_out.c b/net/lapb/lapb_out.c
index 7a4d0715d1c3..a966d29c772d 100644
--- a/net/lapb/lapb_out.c
+++ b/net/lapb/lapb_out.c
@@ -82,7 +82,8 @@ void lapb_kick(struct lapb_cb *lapb)
skb = skb_dequeue(>write_queue);
do {
- if ((skbn = skb_clone(skb, GFP_ATOMIC)) == NULL) {
+ skbn = skb_copy(skb, GFP_ATOMIC);
+ if (!skbn) {
skb_queue_head(>write_queue, skb);
break;
}
--
2.27.0
Re: [PATCH net] net: hdlc_x25: Use qdisc to queue outgoing LAPB frames
On Sat, Jan 30, 2021 at 11:16 AM Jakub Kicinski wrote: > > Sounds like too much afford for a sub-optimal workaround. > The qdisc semantics are borken in the proposed scheme (double > counting packets) - both in term of statistics and if user decides > to add a policer, filter etc. Hmm... Another solution might be creating another virtual device on top of the HDLC device (similar to what "hdlc_fr.c" does), so that we can first queue L3 packets in the virtual device's qdisc queue, and then queue the L2 frames in the actual HDLC device's qdisc queue. This way we can avoid the same outgoing data being queued to qdisc twice. But this would significantly change the way the user uses the hdlc_x25 driver. > Another worry is that something may just inject a packet with > skb->protocol == ETH_P_HDLC but unexpected structure (IDK if > that's a real concern). This might not be a problem. Ethernet devices also allow the user to inject raw frames with user constructed headers. "hdlc_fr.c" also allows the user to bypass the virtual circuit interfaces and inject raw frames directly on the HDLC interface. I think the receiving side should be able to recognize and drop invalid frames. > It may be better to teach LAPB to stop / start the internal queue. > The lower level drivers just needs to call LAPB instead of making > the start/wake calls directly to the stack, and LAPB can call the > stack. Would that not work? I think this is a good solution. But this requires changing a lot of code. The HDLC subsystem needs to be changed to allow HDLC Hardware Drivers to ask HDLC Protocol Drivers (like hdlc_x25.c) to stop/wake the TX queue. The hdlc_x25.c driver can then ask the LAPB module to stop/wake the queue. So this means new APIs need to be added to both the HDLC subsystem and the LAPB module, and a number of HDLC Hardware Drivers need to be changed to call the new API of the HDLC subsystem. Martin, do you have any suggestions?
Re: [PATCH net] net: hdlc_x25: Use qdisc to queue outgoing LAPB frames
On Fri, Jan 29, 2021 at 5:36 PM Jakub Kicinski wrote: > > I'm still struggling to wrap my head around this. > > Did you test your code with lockdep enabled? Which Qdisc are you using? > You're queuing the frames back to the interface they came from - won't > that cause locking issues? Hmm... Thanks for bringing this to my attention. I indeed find issues when the "noqueue" qdisc is used. When using a qdisc other than "noqueue", when sending an skb: "__dev_queue_xmit" will call "__dev_xmit_skb"; "__dev_xmit_skb" will call "qdisc_run_begin" to mark the beginning of a qdisc run, and if the qdisc is already running, "qdisc_run_begin" will fail, then "__dev_xmit_skb" will just enqueue this skb without starting qdisc. There is no problem. When using "noqueue" as the qdisc, when sending an skb: "__dev_queue_xmit" will try to send this skb directly. Before it does that, it will first check "txq->xmit_lock_owner" and will find that the current cpu already owns the xmit lock, it will then print a warning message "Dead loop on virtual device ..." and drop the skb. A solution can be queuing the outgoing L2 frames in this driver first, and then using a tasklet to send them to the qdisc TX queue. Thanks! I'll make changes to fix this.
Re: [PATCH net] net: hdlc_x25: Use qdisc to queue outgoing LAPB frames
On Thu, Jan 28, 2021 at 11:47 AM Jakub Kicinski wrote: > > Noob question - could you point at or provide a quick guide to layering > here? I take there is only one netdev, and something maintains an > internal queue which is not stopped when HW driver stops the qdisc? Yes, there is only one netdev. The LAPB module (net/lapb/) (which is used as a library by the netdev driver - hdlc_x25.c) is maintaining an internal queue which is not stopped when the HW driver stops the qdisc. The queue is "write_queue" in "struct lapb_cb" in "include/net/lapb.h". The code that takes skbs out of the queue and feeds them to lower layers for transmission is at the "lapb_kick" function in "net/lapb/lapb_out.c". The layering is like this: Upper layer (Layer 3) (net/x25/ or net/packet/) ^ | L3 packets (with control info) v The netdev driver (hdlc_x25.c) ^ | L3 packets v The LAPB Module (net/lapb/) ^ | LAPB (L2) frames v The netdev driver (hdlc_x25.c) ^ | LAPB (L2) frames | (also called HDLC frames in the context of the HDLC subsystem) v HDLC core (hdlc.c) ^ | HDLC frames v HDLC Hardware Driver > Sounds like we're optimizing to prevent drops, and this was not > reported from production, rather thru code inspection. Ergo I think > net-next will be more appropriate here, unless Martin disagrees. Yes, I have no problem in targeting net-next instead. Thanks!
Re: [PATCH net] net: hdlc_x25: Use qdisc to queue outgoing LAPB frames
On Wed, Jan 27, 2021 at 2:14 AM David Laight wrote: > > If I read this correctly it adds a (potentially big) queue between the > LAPB code that adds the sequence numbers to the frames and the hardware > that actually sends them. Yes. The actual number of outgoing LAPB frames being queued depends on how long the hardware driver stays in the TX busy state, and is limited by the LAPB sending window. > IIRC [1] there is a general expectation that the NR in a transmitted frame > will be the same as the last received NS unless acks are being delayed > for flow control reasons. > > You definitely want to be able to ack a received frame while transmitting > back-to-back I-frames. > > This really means that you only want 2 frames in the hardware driver. > The one being transmitted and the next one - so it gets sent with a > shared flag. > There is no point sending an RR unless the hardware link is actually idle. If I understand correctly, what you mean is that the frames sent on the wire should reflect the most up-to-date status of what is received from the wire, so queueing outgoing LAPB frames is not appropriate. But this would require us to deal with the "TX busy" issue in the LAPB module. This is (as I said) not easy to do. I currently can't think of a good way of doing this. Instead, we can think of the TX queue as part of the "wire". We can think of the wire as long and having a little higher latency. I believe the LAPB protocol has no problem in handling long wires. What do you think?
[PATCH net] net: hdlc_x25: Use qdisc to queue outgoing LAPB frames
An HDLC hardware driver may call netif_stop_queue to temporarily stop
the TX queue when the hardware is busy sending a frame, and after the
hardware has finished sending the frame, call netif_wake_queue to
resume the TX queue.
However, the LAPB module doesn't know about this. Whether or not the
hardware driver has stopped the TX queue, the LAPB module still feeds
outgoing frames to the hardware driver for transmission. This can cause
frames to be dropped by the hardware driver.
It's not easy to fix this issue in the LAPB module. We can indeed let the
LAPB module check whether the TX queue has been stopped before feeding
each frame to the hardware driver, but when the hardware driver resumes
the TX queue, it's not easy to immediately notify the LAPB module and ask
it to resume transmission.
Instead, we can fix this issue at the hdlc_x25 layer, by using qdisc TX
queues to queue outgoing LAPB frames. The qdisc TX queue will then
automatically be controlled by netif_stop_queue and netif_wake_queue.
This way, when sending, we will use the qdisc queue to queue and send
the data twice: once as the L3 packet and then (after processed by the
LAPB module) as an LAPB (L2) frame. This does not make the logic of the
code messy, because when receiving, data are already "received" on the
device twice: once as an LAPB (L2) frame and then (after processed by
the LAPB module) as the L3 packet.
Some more details about the code change:
1. dev_queue_xmit_nit is removed because we already have it when we send
the skb through the qdisc TX queue (in xmit_one).
2. hdlc_type_trans is replaced by assigning skb->dev and skb->protocol
directly. skb_reset_mac_header in hdlc_type_trans is no longer necessary
because it will be called in __dev_queue_xmit.
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: Martin Schiller
Cc: Krzysztof Halasa
Signed-off-by: Xie He
---
drivers/net/wan/hdlc_x25.c | 17 +
1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/drivers/net/wan/hdlc_x25.c b/drivers/net/wan/hdlc_x25.c
index bb164805804e..b7f2823bf100 100644
--- a/drivers/net/wan/hdlc_x25.c
+++ b/drivers/net/wan/hdlc_x25.c
@@ -89,15 +89,10 @@ static int x25_data_indication(struct net_device *dev,
struct sk_buff *skb)
static void x25_data_transmit(struct net_device *dev, struct sk_buff *skb)
{
- hdlc_device *hdlc = dev_to_hdlc(dev);
-
+ skb->dev = dev;
+ skb->protocol = htons(ETH_P_HDLC);
skb_reset_network_header(skb);
- skb->protocol = hdlc_type_trans(skb, dev);
-
- if (dev_nit_active(dev))
- dev_queue_xmit_nit(skb, dev);
-
- hdlc->xmit(skb, dev); /* Ignore return value :-( */
+ dev_queue_xmit(skb);
}
@@ -106,6 +101,12 @@ static netdev_tx_t x25_xmit(struct sk_buff *skb, struct
net_device *dev)
{
int result;
+ if (skb->protocol == htons(ETH_P_HDLC)) {
+ hdlc_device *hdlc = dev_to_hdlc(dev);
+
+ return hdlc->xmit(skb, dev);
+ }
+
/* There should be a pseudo header of 1 byte added by upper layers.
* Check to make sure it is there before reading it.
*/
--
2.27.0
[PATCH net v6] net: lapb: Add locking to the lapb module
In the lapb module, the timers may run concurrently with other code in
this module, and there is currently no locking to prevent the code from
racing on "struct lapb_cb". This patch adds locking to prevent racing.
1. Add "spinlock_t lock" to "struct lapb_cb"; Add "spin_lock_bh" and
"spin_unlock_bh" to APIs, timer functions and notifier functions.
2. Add "bool t1timer_stop, t2timer_stop" to "struct lapb_cb" to make us
able to ask running timers to abort; Modify "lapb_stop_t1timer" and
"lapb_stop_t2timer" to make them able to abort running timers;
Modify "lapb_t2timer_expiry" and "lapb_t1timer_expiry" to make them
abort after they are stopped by "lapb_stop_t1timer", "lapb_stop_t2timer",
and "lapb_start_t1timer", "lapb_start_t2timer".
3. Let lapb_unregister wait for other API functions and running timers
to stop.
4. The lapb_device_event function calls lapb_disconnect_request. In
order to avoid trying to hold the lock twice, add a new function named
"__lapb_disconnect_request" which assumes the lock is held, and make
it called by lapb_disconnect_request and lapb_device_event.
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: Martin Schiller
Signed-off-by: Xie He
---
Change from v5:
Add usleep_range to the waiting loop in lapb_unregister.
I didn't restore the "goto"s in "__lapb_disconnect_request" because
after the function splitting there's nowhere we can "goto" anymore.
Change from v4:
Make lapb_unregister wait for other refs to "lapb" to drop, to ensure
that other LAPB API calls have all finished.
Change from v3:
In lapb_unregister make sure the self-restarting t1timer has really been
stopped.
Change from v2:
Create a new __lapb_disconnect_request function to reduce redundant code.
Change from v1:
Break long lines to keep the line lengths within 80 characters.
---
include/net/lapb.h| 2 ++
net/lapb/lapb_iface.c | 70 +--
net/lapb/lapb_timer.c | 30 ---
3 files changed, 82 insertions(+), 20 deletions(-)
diff --git a/include/net/lapb.h b/include/net/lapb.h
index ccc3d1f020b0..eee73442a1ba 100644
--- a/include/net/lapb.h
+++ b/include/net/lapb.h
@@ -92,6 +92,7 @@ struct lapb_cb {
unsigned short n2, n2count;
unsigned short t1, t2;
struct timer_list t1timer, t2timer;
+ boolt1timer_stop, t2timer_stop;
/* Internal control information */
struct sk_buff_head write_queue;
@@ -103,6 +104,7 @@ struct lapb_cb {
struct lapb_frame frmr_data;
unsigned char frmr_type;
+ spinlock_t lock;
refcount_t refcnt;
};
diff --git a/net/lapb/lapb_iface.c b/net/lapb/lapb_iface.c
index 40961889e9c0..0511bbe4af7b 100644
--- a/net/lapb/lapb_iface.c
+++ b/net/lapb/lapb_iface.c
@@ -122,6 +122,8 @@ static struct lapb_cb *lapb_create_cb(void)
timer_setup(>t1timer, NULL, 0);
timer_setup(>t2timer, NULL, 0);
+ lapb->t1timer_stop = true;
+ lapb->t2timer_stop = true;
lapb->t1 = LAPB_DEFAULT_T1;
lapb->t2 = LAPB_DEFAULT_T2;
@@ -129,6 +131,8 @@ static struct lapb_cb *lapb_create_cb(void)
lapb->mode= LAPB_DEFAULT_MODE;
lapb->window = LAPB_DEFAULT_WINDOW;
lapb->state = LAPB_STATE_0;
+
+ spin_lock_init(>lock);
refcount_set(>refcnt, 1);
out:
return lapb;
@@ -178,11 +182,23 @@ int lapb_unregister(struct net_device *dev)
goto out;
lapb_put(lapb);
+ /* Wait for other refs to "lapb" to drop */
+ while (refcount_read(>refcnt) > 2)
+ usleep_range(1, 10);
+
+ spin_lock_bh(>lock);
+
lapb_stop_t1timer(lapb);
lapb_stop_t2timer(lapb);
lapb_clear_queues(lapb);
+ spin_unlock_bh(>lock);
+
+ /* Wait for running timers to stop */
+ del_timer_sync(>t1timer);
+ del_timer_sync(>t2timer);
+
__lapb_remove_cb(lapb);
lapb_put(lapb);
@@ -201,6 +217,8 @@ int lapb_getparms(struct net_device *dev, struct
lapb_parms_struct *parms)
if (!lapb)
goto out;
+ spin_lock_bh(>lock);
+
parms->t1 = lapb->t1 / HZ;
parms->t2 = lapb->t2 / HZ;
parms->n2 = lapb->n2;
@@ -219,6 +237,7 @@ int lapb_getparms(struct net_device *dev, struct
lapb_parms_struct *parms)
else
parms->t2timer = (lapb->t2timer.expires - jiffies) / HZ;
+ spin_unlock_bh(>lock);
lapb_put(lapb);
rc = LAPB_OK;
out:
@@ -234,6 +253,8 @@ int lapb_setparms(struct net_device *dev, struct
lapb_parms_struct *parms)
if (!lapb)
Re: [PATCH net v5] net: lapb: Add locking to the lapb module
On Sat, Jan 23, 2021 at 8:45 PM Jakub Kicinski wrote:
>
> > > @@ -178,11 +182,23 @@ int lapb_unregister(struct net_device *dev)
> > > goto out;
> > > lapb_put(lapb);
> > >
> > > + /* Wait for other refs to "lapb" to drop */
> > > + while (refcount_read(>refcnt) > 2)
> > > + ;
>
> Tight loop like this is a little scary, perhaps add a small
> usleep_range() here?
OK, sure. I'll add a usleep_range(1, 10) here.
> > > -int lapb_disconnect_request(struct net_device *dev)
> > > +static int __lapb_disconnect_request(struct lapb_cb *lapb)
> > > {
> > > - struct lapb_cb *lapb = lapb_devtostruct(dev);
> > > - int rc = LAPB_BADTOKEN;
> > > -
> > > - if (!lapb)
> > > - goto out;
> > > -
> > > switch (lapb->state) {
> > > case LAPB_STATE_0:
> > > - rc = LAPB_NOTCONNECTED;
> > > - goto out_put;
> > > + return LAPB_NOTCONNECTED;
> > >
> > > case LAPB_STATE_1:
> > > lapb_dbg(1, "(%p) S1 TX DISC(1)\n", lapb->dev);
> > > @@ -310,12 +328,10 @@ int lapb_disconnect_request(struct net_device
> > > *dev)
> > > lapb_send_control(lapb, LAPB_DISC, LAPB_POLLON, LAPB_COMMAND);
> > > lapb->state = LAPB_STATE_0;
> > > lapb_start_t1timer(lapb);
> > > - rc = LAPB_NOTCONNECTED;
> > > - goto out_put;
> > > + return LAPB_NOTCONNECTED;
> > >
> > > case LAPB_STATE_2:
> > > - rc = LAPB_OK;
> > > - goto out_put;
> > > + return LAPB_OK;
> > > }
> > >
> > > lapb_clear_queues(lapb);
> > > @@ -328,8 +344,22 @@ int lapb_disconnect_request(struct net_device
> > > *dev)
> > > lapb_dbg(1, "(%p) S3 DISC(1)\n", lapb->dev);
> > > lapb_dbg(0, "(%p) S3 -> S2\n", lapb->dev);
> > >
> > > - rc = LAPB_OK;
> > > -out_put:
> > > + return LAPB_OK;
> > > +}
>
> Since this is a fix for net, I'd advise against converting the goto
> into direct returns (as much as I generally like such conversion).
This part is actually splitting "lapb_disconnect_request" into two
functions - a "__lapb_disconnect_request" without locking, and a
"lapb_disconnect_request" which provides the locking and calls
"__lapb_disconnect_request". The splitting is necessary for
"lapb_device_event" to directly call "__lapb_disconnect_request" with
the lock already held. After the splitting, the "out_put" tag would
actually be in the caller function so there's nowhere we can "goto".
Re: [PATCH net v5] net: lapb: Add locking to the lapb module
On Fri, Jan 22, 2021 at 1:07 AM Martin Schiller wrote: > > I don't have the opportunity to test this at the moment, but code looks > reasonable so far. Have you tested this at runtime? Thanks! Yes, I have tested this using hdlc_x25.c, lapbether.c and (the deleted) x25_asy.c drivers.
Re: [PATCH net v4] net: lapb: Add locking to the lapb module
On Wed, Jan 20, 2021 at 12:42 PM Xie He wrote: > > With this patch, there is still a problem that lapb_unregister may run > concurrently with other LAPB API functions (such as > lapb_data_received). This other LAPB API function can get the > lapb->lock after lapb->lock is released by lapb_unregister, and > continue to do its work. This is not correct. > > We can fix this problem by adding a new field "bool stop" to "struct > lapb_cb" (just like "bool t1timer_stop, t2timer_stop"), and make every > API function abort whenever it sees lapb->stop == true after getting > the lock. > > Alternatively we can also require the callers (the LAPB drivers) to > never call lapb_unregister concurrently with other LAPB APIs. They > should make sure all LAPB API functions are only called after > lapb_register ends and before lapb_unregister starts. This is a > reasonable requirement, because if they don't follow this requirement, > even if we do the fix in the LAPB module (as said above), the LAPB > driver will still get the "LAPB_BADTOKEN" error from the LAPB module. > This is not desirable and I think LAPB drivers should avoid this from > happening. > > So I think this problem may not need to be fixed here in the LAPB > module because the LAPB drivers should deal with this problem anyway. Never mind, I have sent a v5 to deal with this problem. In v5, I made lapb_unregister wait for the "lapb" refcnt to drop, so that we can make sure all other API calls have finished. Please see my v5.
[PATCH net v5] net: lapb: Add locking to the lapb module
In the lapb module, the timers may run concurrently with other code in
this module, and there is currently no locking to prevent the code from
racing on "struct lapb_cb". This patch adds locking to prevent racing.
1. Add "spinlock_t lock" to "struct lapb_cb"; Add "spin_lock_bh" and
"spin_unlock_bh" to APIs, timer functions and notifier functions.
2. Add "bool t1timer_stop, t2timer_stop" to "struct lapb_cb" to make us
able to ask running timers to abort; Modify "lapb_stop_t1timer" and
"lapb_stop_t2timer" to make them able to abort running timers;
Modify "lapb_t2timer_expiry" and "lapb_t1timer_expiry" to make them
abort after they are stopped by "lapb_stop_t1timer", "lapb_stop_t2timer",
and "lapb_start_t1timer", "lapb_start_t2timer".
3. Let lapb_unregister wait for other API functions and running timers
to stop.
4. The lapb_device_event function calls lapb_disconnect_request. In
order to avoid trying to hold the lock twice, add a new function named
"__lapb_disconnect_request" which assumes the lock is held, and make
it called by lapb_disconnect_request and lapb_device_event.
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: Martin Schiller
Signed-off-by: Xie He
---
Change from v4:
Make lapb_unregister wait for other refs to "lapb" to drop, to ensure
that other LAPB API calls have all finished.
Change from v3:
In lapb_unregister make sure the self-restarting t1timer has really been
stopped.
Change from v2:
Create a new __lapb_disconnect_request function to reduce redundant code.
Change from v1:
Broke long lines to keep the line lengths within 80 characters.
---
include/net/lapb.h| 2 ++
net/lapb/lapb_iface.c | 70 +--
net/lapb/lapb_timer.c | 30 ---
3 files changed, 82 insertions(+), 20 deletions(-)
diff --git a/include/net/lapb.h b/include/net/lapb.h
index ccc3d1f020b0..eee73442a1ba 100644
--- a/include/net/lapb.h
+++ b/include/net/lapb.h
@@ -92,6 +92,7 @@ struct lapb_cb {
unsigned short n2, n2count;
unsigned short t1, t2;
struct timer_list t1timer, t2timer;
+ boolt1timer_stop, t2timer_stop;
/* Internal control information */
struct sk_buff_head write_queue;
@@ -103,6 +104,7 @@ struct lapb_cb {
struct lapb_frame frmr_data;
unsigned char frmr_type;
+ spinlock_t lock;
refcount_t refcnt;
};
diff --git a/net/lapb/lapb_iface.c b/net/lapb/lapb_iface.c
index 40961889e9c0..b028dfc438ce 100644
--- a/net/lapb/lapb_iface.c
+++ b/net/lapb/lapb_iface.c
@@ -122,6 +122,8 @@ static struct lapb_cb *lapb_create_cb(void)
timer_setup(>t1timer, NULL, 0);
timer_setup(>t2timer, NULL, 0);
+ lapb->t1timer_stop = true;
+ lapb->t2timer_stop = true;
lapb->t1 = LAPB_DEFAULT_T1;
lapb->t2 = LAPB_DEFAULT_T2;
@@ -129,6 +131,8 @@ static struct lapb_cb *lapb_create_cb(void)
lapb->mode= LAPB_DEFAULT_MODE;
lapb->window = LAPB_DEFAULT_WINDOW;
lapb->state = LAPB_STATE_0;
+
+ spin_lock_init(>lock);
refcount_set(>refcnt, 1);
out:
return lapb;
@@ -178,11 +182,23 @@ int lapb_unregister(struct net_device *dev)
goto out;
lapb_put(lapb);
+ /* Wait for other refs to "lapb" to drop */
+ while (refcount_read(>refcnt) > 2)
+ ;
+
+ spin_lock_bh(>lock);
+
lapb_stop_t1timer(lapb);
lapb_stop_t2timer(lapb);
lapb_clear_queues(lapb);
+ spin_unlock_bh(>lock);
+
+ /* Wait for running timers to stop */
+ del_timer_sync(>t1timer);
+ del_timer_sync(>t2timer);
+
__lapb_remove_cb(lapb);
lapb_put(lapb);
@@ -201,6 +217,8 @@ int lapb_getparms(struct net_device *dev, struct
lapb_parms_struct *parms)
if (!lapb)
goto out;
+ spin_lock_bh(>lock);
+
parms->t1 = lapb->t1 / HZ;
parms->t2 = lapb->t2 / HZ;
parms->n2 = lapb->n2;
@@ -219,6 +237,7 @@ int lapb_getparms(struct net_device *dev, struct
lapb_parms_struct *parms)
else
parms->t2timer = (lapb->t2timer.expires - jiffies) / HZ;
+ spin_unlock_bh(>lock);
lapb_put(lapb);
rc = LAPB_OK;
out:
@@ -234,6 +253,8 @@ int lapb_setparms(struct net_device *dev, struct
lapb_parms_struct *parms)
if (!lapb)
goto out;
+ spin_lock_bh(>lock);
+
rc = LAPB_INVALUE;
if (parms->t1 < 1 || parms->t2 < 1 || parms->n2 < 1)
goto out_put;
@@ -256,6 +277,7 @@ int lapb_setparms(struct net_device *dev, struct
lapb_
Re: [PATCH net v4] net: lapb: Add locking to the lapb module
With this patch, there is still a problem that lapb_unregister may run concurrently with other LAPB API functions (such as lapb_data_received). This other LAPB API function can get the lapb->lock after lapb->lock is released by lapb_unregister, and continue to do its work. This is not correct. We can fix this problem by adding a new field "bool stop" to "struct lapb_cb" (just like "bool t1timer_stop, t2timer_stop"), and make every API function abort whenever it sees lapb->stop == true after getting the lock. Alternatively we can also require the callers (the LAPB drivers) to never call lapb_unregister concurrently with other LAPB APIs. They should make sure all LAPB API functions are only called after lapb_register ends and before lapb_unregister starts. This is a reasonable requirement, because if they don't follow this requirement, even if we do the fix in the LAPB module (as said above), the LAPB driver will still get the "LAPB_BADTOKEN" error from the LAPB module. This is not desirable and I think LAPB drivers should avoid this from happening. So I think this problem may not need to be fixed here in the LAPB module because the LAPB drivers should deal with this problem anyway. Please feel free to share your comment. Thanks!
Re: [PATCH net v4] net: lapb: Add locking to the lapb module
On Wed, Jan 20, 2021 at 2:58 AM Martin Schiller wrote: > > Can you please add a Changelog. What was changed in v4? Sorry, I forgot this. Here is the change log: --- Changes from v3 to v4 --- Only lapb_unregister has been changed. v3 has a problem. When "del_timer_sync(>t1timer)" is called, if the t1timer is running, it may restart itself by calling lapb_start_t1timer. This way, del_timer_sync would not be able to guarantee the t1timer has been completely stopped. v4 fixed this problem by first calling lapb_stop_t1timer, making use of its (new) ability of aborting running timers, and then calling del_timer_sync to guarantee the t1timer has been stopped. --- Changes from v2 to v3 --- Created a new __lapb_disconnect_request function and made it be called from both lapb_disconnect_request and lapb_device_event. This reduced redundant code. --- Changes from v1 to v2 --- Broke long lines to keep the line lengths within 80 characters.
[PATCH net v4] net: lapb: Add locking to the lapb module
In the lapb module, the timers may run concurrently with other code in
this module, and there is currently no locking to prevent the code from
racing on "struct lapb_cb". This patch adds locking to prevent racing.
1. Add "spinlock_t lock" to "struct lapb_cb"; Add "spin_lock_bh" and
"spin_unlock_bh" to APIs, timer functions and notifier functions.
2. Add "bool t1timer_stop, t2timer_stop" to "struct lapb_cb" to make us
able to ask running timers to abort; Modify "lapb_stop_t1timer" and
"lapb_stop_t2timer" to make them able to abort running timers;
Modify "lapb_t2timer_expiry" and "lapb_t1timer_expiry" to make them
abort after they are stopped by "lapb_stop_t1timer", "lapb_stop_t2timer",
and "lapb_start_t1timer", "lapb_start_t2timer".
3. In lapb_unregister, add "del_timer_sync" calls to make sure all
running timers have exited.
4. The lapb_device_event function calls lapb_disconnect_request. In
order to avoid trying to hold the lock twice, add a new function named
"__lapb_disconnect_request" which assumes the lock is held, and make
it called by lapb_disconnect_request and lapb_device_event.
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: Martin Schiller
Signed-off-by: Xie He
---
include/net/lapb.h| 2 ++
net/lapb/lapb_iface.c | 65 ---
net/lapb/lapb_timer.c | 30 +---
3 files changed, 77 insertions(+), 20 deletions(-)
diff --git a/include/net/lapb.h b/include/net/lapb.h
index ccc3d1f020b0..eee73442a1ba 100644
--- a/include/net/lapb.h
+++ b/include/net/lapb.h
@@ -92,6 +92,7 @@ struct lapb_cb {
unsigned short n2, n2count;
unsigned short t1, t2;
struct timer_list t1timer, t2timer;
+ boolt1timer_stop, t2timer_stop;
/* Internal control information */
struct sk_buff_head write_queue;
@@ -103,6 +104,7 @@ struct lapb_cb {
struct lapb_frame frmr_data;
unsigned char frmr_type;
+ spinlock_t lock;
refcount_t refcnt;
};
diff --git a/net/lapb/lapb_iface.c b/net/lapb/lapb_iface.c
index 40961889e9c0..205f8736e68b 100644
--- a/net/lapb/lapb_iface.c
+++ b/net/lapb/lapb_iface.c
@@ -122,6 +122,8 @@ static struct lapb_cb *lapb_create_cb(void)
timer_setup(>t1timer, NULL, 0);
timer_setup(>t2timer, NULL, 0);
+ lapb->t1timer_stop = true;
+ lapb->t2timer_stop = true;
lapb->t1 = LAPB_DEFAULT_T1;
lapb->t2 = LAPB_DEFAULT_T2;
@@ -129,6 +131,8 @@ static struct lapb_cb *lapb_create_cb(void)
lapb->mode= LAPB_DEFAULT_MODE;
lapb->window = LAPB_DEFAULT_WINDOW;
lapb->state = LAPB_STATE_0;
+
+ spin_lock_init(>lock);
refcount_set(>refcnt, 1);
out:
return lapb;
@@ -178,11 +182,18 @@ int lapb_unregister(struct net_device *dev)
goto out;
lapb_put(lapb);
+ spin_lock_bh(>lock);
+
lapb_stop_t1timer(lapb);
lapb_stop_t2timer(lapb);
lapb_clear_queues(lapb);
+ spin_unlock_bh(>lock);
+
+ del_timer_sync(>t1timer);
+ del_timer_sync(>t2timer);
+
__lapb_remove_cb(lapb);
lapb_put(lapb);
@@ -201,6 +212,8 @@ int lapb_getparms(struct net_device *dev, struct
lapb_parms_struct *parms)
if (!lapb)
goto out;
+ spin_lock_bh(>lock);
+
parms->t1 = lapb->t1 / HZ;
parms->t2 = lapb->t2 / HZ;
parms->n2 = lapb->n2;
@@ -219,6 +232,7 @@ int lapb_getparms(struct net_device *dev, struct
lapb_parms_struct *parms)
else
parms->t2timer = (lapb->t2timer.expires - jiffies) / HZ;
+ spin_unlock_bh(>lock);
lapb_put(lapb);
rc = LAPB_OK;
out:
@@ -234,6 +248,8 @@ int lapb_setparms(struct net_device *dev, struct
lapb_parms_struct *parms)
if (!lapb)
goto out;
+ spin_lock_bh(>lock);
+
rc = LAPB_INVALUE;
if (parms->t1 < 1 || parms->t2 < 1 || parms->n2 < 1)
goto out_put;
@@ -256,6 +272,7 @@ int lapb_setparms(struct net_device *dev, struct
lapb_parms_struct *parms)
rc = LAPB_OK;
out_put:
+ spin_unlock_bh(>lock);
lapb_put(lapb);
out:
return rc;
@@ -270,6 +287,8 @@ int lapb_connect_request(struct net_device *dev)
if (!lapb)
goto out;
+ spin_lock_bh(>lock);
+
rc = LAPB_OK;
if (lapb->state == LAPB_STATE_1)
goto out_put;
@@ -285,24 +304,18 @@ int lapb_connect_request(struct net_device *dev)
rc = LAPB_OK;
out_put:
+ spin_unlock_bh(>lock);
lapb_put(lapb);
out:
return rc
Re: [PATCH net v2] net: lapb: Add locking to the lapb module
On Tue, Jan 19, 2021 at 3:34 AM Martin Schiller wrote: > > > 4. In lapb_device_event, replace the "lapb_disconnect_request" call > > with > > the content of "lapb_disconnect_request", to avoid trying to hold the > > lock twice. When I do this, I removed "lapb_start_t1timer" because I > > don't think it's necessary to start the timer when "NETDEV_GOING_DOWN". > > I don't like the code redundancy this creates. Maybe you should move the > actual functionality from lapb_disconnect_request() to a > __lapb_disconnect_request(), and in lapb_disconnect_request() call this > function including locking around it and also in lapb_device_event > (without locking). > > Calling lapb_start_t1timer() on a "NETDEV_GOING_DOWN" event does not > hurt and is correct from a protocol flow point of view after sending > the DISC. Thanks! I created a new __lapb_disconnect_request function and the code indeed looked cleaner. I'll send a new version.
[PATCH net v3] net: lapb: Add locking to the lapb module
In the lapb module, the timers may run concurrently with other code in
this module, and there is currently no locking to prevent the code from
racing on "struct lapb_cb". This patch adds locking to prevent racing.
1. Add "spinlock_t lock" to "struct lapb_cb"; Add "spin_lock_bh" and
"spin_unlock_bh" to APIs, timer functions and notifier functions.
2. Add "bool t1timer_stop, t2timer_stop" to "struct lapb_cb" to make us
able to ask running timers to abort; Modify "lapb_stop_t1timer" and
"lapb_stop_t2timer" to make them able to abort running timers;
Modify "lapb_t2timer_expiry" and "lapb_t1timer_expiry" to make them
abort after they are stopped by "lapb_stop_t1timer", "lapb_stop_t2timer",
and "lapb_start_t1timer", "lapb_start_t2timer".
3. In lapb_unregister, change "lapb_stop_t1timer" and "lapb_stop_t2timer"
to "del_timer_sync" to make sure all running timers have exited.
4. The lapb_device_event function calls lapb_disconnect_request. In
order to avoid trying to hold the lock twice, add a new function named
"__lapb_disconnect_request" which assumes the lock is held, and make
it called by lapb_disconnect_request and lapb_device_event.
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: Martin Schiller
Signed-off-by: Xie He
---
include/net/lapb.h| 2 ++
net/lapb/lapb_iface.c | 62 ++-
net/lapb/lapb_timer.c | 30 ++---
3 files changed, 72 insertions(+), 22 deletions(-)
diff --git a/include/net/lapb.h b/include/net/lapb.h
index ccc3d1f020b0..eee73442a1ba 100644
--- a/include/net/lapb.h
+++ b/include/net/lapb.h
@@ -92,6 +92,7 @@ struct lapb_cb {
unsigned short n2, n2count;
unsigned short t1, t2;
struct timer_list t1timer, t2timer;
+ boolt1timer_stop, t2timer_stop;
/* Internal control information */
struct sk_buff_head write_queue;
@@ -103,6 +104,7 @@ struct lapb_cb {
struct lapb_frame frmr_data;
unsigned char frmr_type;
+ spinlock_t lock;
refcount_t refcnt;
};
diff --git a/net/lapb/lapb_iface.c b/net/lapb/lapb_iface.c
index 40961889e9c0..4de2edbe833a 100644
--- a/net/lapb/lapb_iface.c
+++ b/net/lapb/lapb_iface.c
@@ -122,6 +122,8 @@ static struct lapb_cb *lapb_create_cb(void)
timer_setup(>t1timer, NULL, 0);
timer_setup(>t2timer, NULL, 0);
+ lapb->t1timer_stop = true;
+ lapb->t2timer_stop = true;
lapb->t1 = LAPB_DEFAULT_T1;
lapb->t2 = LAPB_DEFAULT_T2;
@@ -129,6 +131,8 @@ static struct lapb_cb *lapb_create_cb(void)
lapb->mode= LAPB_DEFAULT_MODE;
lapb->window = LAPB_DEFAULT_WINDOW;
lapb->state = LAPB_STATE_0;
+
+ spin_lock_init(>lock);
refcount_set(>refcnt, 1);
out:
return lapb;
@@ -178,8 +182,8 @@ int lapb_unregister(struct net_device *dev)
goto out;
lapb_put(lapb);
- lapb_stop_t1timer(lapb);
- lapb_stop_t2timer(lapb);
+ del_timer_sync(>t1timer);
+ del_timer_sync(>t2timer);
lapb_clear_queues(lapb);
@@ -201,6 +205,8 @@ int lapb_getparms(struct net_device *dev, struct
lapb_parms_struct *parms)
if (!lapb)
goto out;
+ spin_lock_bh(>lock);
+
parms->t1 = lapb->t1 / HZ;
parms->t2 = lapb->t2 / HZ;
parms->n2 = lapb->n2;
@@ -219,6 +225,7 @@ int lapb_getparms(struct net_device *dev, struct
lapb_parms_struct *parms)
else
parms->t2timer = (lapb->t2timer.expires - jiffies) / HZ;
+ spin_unlock_bh(>lock);
lapb_put(lapb);
rc = LAPB_OK;
out:
@@ -234,6 +241,8 @@ int lapb_setparms(struct net_device *dev, struct
lapb_parms_struct *parms)
if (!lapb)
goto out;
+ spin_lock_bh(>lock);
+
rc = LAPB_INVALUE;
if (parms->t1 < 1 || parms->t2 < 1 || parms->n2 < 1)
goto out_put;
@@ -256,6 +265,7 @@ int lapb_setparms(struct net_device *dev, struct
lapb_parms_struct *parms)
rc = LAPB_OK;
out_put:
+ spin_unlock_bh(>lock);
lapb_put(lapb);
out:
return rc;
@@ -270,6 +280,8 @@ int lapb_connect_request(struct net_device *dev)
if (!lapb)
goto out;
+ spin_lock_bh(>lock);
+
rc = LAPB_OK;
if (lapb->state == LAPB_STATE_1)
goto out_put;
@@ -285,24 +297,18 @@ int lapb_connect_request(struct net_device *dev)
rc = LAPB_OK;
out_put:
+ spin_unlock_bh(>lock);
lapb_put(lapb);
out:
return rc;
}
EXPORT_SYMBOL(lapb_connect_request);
-int
[PATCH net v2] net: lapb: Add locking to the lapb module
In the lapb module, the timers may run concurrently with other code in
this module, and there is currently no locking to prevent the code from
racing on "struct lapb_cb". This patch adds locking to prevent racing.
1. Add "spinlock_t lock" to "struct lapb_cb"; Add "spin_lock_bh" and
"spin_unlock_bh" to APIs, timer functions and notifier functions.
2. Add "bool t1timer_stop, t2timer_stop" to "struct lapb_cb" to make us
able to ask running timers to abort; Modify "lapb_stop_t1timer" and
"lapb_stop_t2timer" to make them able to abort running timers;
Modify "lapb_t2timer_expiry" and "lapb_t1timer_expiry" to make them
abort after they are stopped by "lapb_stop_t1timer", "lapb_stop_t2timer",
and "lapb_start_t1timer", "lapb_start_t2timer".
3. In lapb_unregister, change "lapb_stop_t1timer" and "lapb_stop_t2timer"
to "del_timer_sync" to make sure all running timers have exited.
4. In lapb_device_event, replace the "lapb_disconnect_request" call with
the content of "lapb_disconnect_request", to avoid trying to hold the
lock twice. When I do this, I removed "lapb_start_t1timer" because I
don't think it's necessary to start the timer when "NETDEV_GOING_DOWN".
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: Martin Schiller
Signed-off-by: Xie He
---
include/net/lapb.h| 2 ++
net/lapb/lapb_iface.c | 56 +++
net/lapb/lapb_timer.c | 30 +++
3 files changed, 80 insertions(+), 8 deletions(-)
diff --git a/include/net/lapb.h b/include/net/lapb.h
index ccc3d1f020b0..eee73442a1ba 100644
--- a/include/net/lapb.h
+++ b/include/net/lapb.h
@@ -92,6 +92,7 @@ struct lapb_cb {
unsigned short n2, n2count;
unsigned short t1, t2;
struct timer_list t1timer, t2timer;
+ boolt1timer_stop, t2timer_stop;
/* Internal control information */
struct sk_buff_head write_queue;
@@ -103,6 +104,7 @@ struct lapb_cb {
struct lapb_frame frmr_data;
unsigned char frmr_type;
+ spinlock_t lock;
refcount_t refcnt;
};
diff --git a/net/lapb/lapb_iface.c b/net/lapb/lapb_iface.c
index 40961889e9c0..45f332607685 100644
--- a/net/lapb/lapb_iface.c
+++ b/net/lapb/lapb_iface.c
@@ -122,6 +122,8 @@ static struct lapb_cb *lapb_create_cb(void)
timer_setup(>t1timer, NULL, 0);
timer_setup(>t2timer, NULL, 0);
+ lapb->t1timer_stop = true;
+ lapb->t2timer_stop = true;
lapb->t1 = LAPB_DEFAULT_T1;
lapb->t2 = LAPB_DEFAULT_T2;
@@ -129,6 +131,8 @@ static struct lapb_cb *lapb_create_cb(void)
lapb->mode= LAPB_DEFAULT_MODE;
lapb->window = LAPB_DEFAULT_WINDOW;
lapb->state = LAPB_STATE_0;
+
+ spin_lock_init(>lock);
refcount_set(>refcnt, 1);
out:
return lapb;
@@ -178,8 +182,8 @@ int lapb_unregister(struct net_device *dev)
goto out;
lapb_put(lapb);
- lapb_stop_t1timer(lapb);
- lapb_stop_t2timer(lapb);
+ del_timer_sync(>t1timer);
+ del_timer_sync(>t2timer);
lapb_clear_queues(lapb);
@@ -201,6 +205,8 @@ int lapb_getparms(struct net_device *dev, struct
lapb_parms_struct *parms)
if (!lapb)
goto out;
+ spin_lock_bh(>lock);
+
parms->t1 = lapb->t1 / HZ;
parms->t2 = lapb->t2 / HZ;
parms->n2 = lapb->n2;
@@ -219,6 +225,7 @@ int lapb_getparms(struct net_device *dev, struct
lapb_parms_struct *parms)
else
parms->t2timer = (lapb->t2timer.expires - jiffies) / HZ;
+ spin_unlock_bh(>lock);
lapb_put(lapb);
rc = LAPB_OK;
out:
@@ -234,6 +241,8 @@ int lapb_setparms(struct net_device *dev, struct
lapb_parms_struct *parms)
if (!lapb)
goto out;
+ spin_lock_bh(>lock);
+
rc = LAPB_INVALUE;
if (parms->t1 < 1 || parms->t2 < 1 || parms->n2 < 1)
goto out_put;
@@ -256,6 +265,7 @@ int lapb_setparms(struct net_device *dev, struct
lapb_parms_struct *parms)
rc = LAPB_OK;
out_put:
+ spin_unlock_bh(>lock);
lapb_put(lapb);
out:
return rc;
@@ -270,6 +280,8 @@ int lapb_connect_request(struct net_device *dev)
if (!lapb)
goto out;
+ spin_lock_bh(>lock);
+
rc = LAPB_OK;
if (lapb->state == LAPB_STATE_1)
goto out_put;
@@ -285,6 +297,7 @@ int lapb_connect_request(struct net_device *dev)
rc = LAPB_OK;
out_put:
+ spin_unlock_bh(>lock);
lapb_put(lapb);
out:
return rc;
@@ -299,6 +312,8 @@
[PATCH net] net: lapb: Add locking to the lapb module
In the lapb module, the timers may run concurrently with other code in
this module, and there is currently no locking to prevent the code from
racing on "struct lapb_cb". This patch adds locking to prevent racing.
1. Add "spinlock_t lock" to "struct lapb_cb"; Add "spin_lock_bh" and
"spin_unlock_bh" to APIs, timer functions and notifier functions.
2. Add "bool t1timer_stop, t2timer_stop" to "struct lapb_cb" to make us
able to ask running timers to abort; Modify "lapb_stop_t1timer" and
"lapb_stop_t2timer" to make them able to abort running timers;
Modify "lapb_t2timer_expiry" and "lapb_t1timer_expiry" to make them
abort after they are stopped by "lapb_stop_t1timer", "lapb_stop_t2timer",
and "lapb_start_t1timer", "lapb_start_t2timer".
3. In lapb_unregister, change "lapb_stop_t1timer" and "lapb_stop_t2timer"
to "del_timer_sync" to make sure all running timers have exited.
4. In lapb_device_event, replace the "lapb_disconnect_request" call with
the content of "lapb_disconnect_request", to avoid trying to hold the
lock twice. When doing this, "lapb_start_t1timer" is removed because
I don't think it's necessary to start the timer when "NETDEV_GOING_DOWN".
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: Martin Schiller
Signed-off-by: Xie He
---
include/net/lapb.h| 2 ++
net/lapb/lapb_iface.c | 54 +++
net/lapb/lapb_timer.c | 30
3 files changed, 78 insertions(+), 8 deletions(-)
diff --git a/include/net/lapb.h b/include/net/lapb.h
index ccc3d1f020b0..eee73442a1ba 100644
--- a/include/net/lapb.h
+++ b/include/net/lapb.h
@@ -92,6 +92,7 @@ struct lapb_cb {
unsigned short n2, n2count;
unsigned short t1, t2;
struct timer_list t1timer, t2timer;
+ boolt1timer_stop, t2timer_stop;
/* Internal control information */
struct sk_buff_head write_queue;
@@ -103,6 +104,7 @@ struct lapb_cb {
struct lapb_frame frmr_data;
unsigned char frmr_type;
+ spinlock_t lock;
refcount_t refcnt;
};
diff --git a/net/lapb/lapb_iface.c b/net/lapb/lapb_iface.c
index 40961889e9c0..ad6197381d05 100644
--- a/net/lapb/lapb_iface.c
+++ b/net/lapb/lapb_iface.c
@@ -122,6 +122,8 @@ static struct lapb_cb *lapb_create_cb(void)
timer_setup(>t1timer, NULL, 0);
timer_setup(>t2timer, NULL, 0);
+ lapb->t1timer_stop = true;
+ lapb->t2timer_stop = true;
lapb->t1 = LAPB_DEFAULT_T1;
lapb->t2 = LAPB_DEFAULT_T2;
@@ -129,6 +131,8 @@ static struct lapb_cb *lapb_create_cb(void)
lapb->mode= LAPB_DEFAULT_MODE;
lapb->window = LAPB_DEFAULT_WINDOW;
lapb->state = LAPB_STATE_0;
+
+ spin_lock_init(>lock);
refcount_set(>refcnt, 1);
out:
return lapb;
@@ -178,8 +182,8 @@ int lapb_unregister(struct net_device *dev)
goto out;
lapb_put(lapb);
- lapb_stop_t1timer(lapb);
- lapb_stop_t2timer(lapb);
+ del_timer_sync(>t1timer);
+ del_timer_sync(>t2timer);
lapb_clear_queues(lapb);
@@ -201,6 +205,8 @@ int lapb_getparms(struct net_device *dev, struct
lapb_parms_struct *parms)
if (!lapb)
goto out;
+ spin_lock_bh(>lock);
+
parms->t1 = lapb->t1 / HZ;
parms->t2 = lapb->t2 / HZ;
parms->n2 = lapb->n2;
@@ -219,6 +225,7 @@ int lapb_getparms(struct net_device *dev, struct
lapb_parms_struct *parms)
else
parms->t2timer = (lapb->t2timer.expires - jiffies) / HZ;
+ spin_unlock_bh(>lock);
lapb_put(lapb);
rc = LAPB_OK;
out:
@@ -234,6 +241,8 @@ int lapb_setparms(struct net_device *dev, struct
lapb_parms_struct *parms)
if (!lapb)
goto out;
+ spin_lock_bh(>lock);
+
rc = LAPB_INVALUE;
if (parms->t1 < 1 || parms->t2 < 1 || parms->n2 < 1)
goto out_put;
@@ -256,6 +265,7 @@ int lapb_setparms(struct net_device *dev, struct
lapb_parms_struct *parms)
rc = LAPB_OK;
out_put:
+ spin_unlock_bh(>lock);
lapb_put(lapb);
out:
return rc;
@@ -270,6 +280,8 @@ int lapb_connect_request(struct net_device *dev)
if (!lapb)
goto out;
+ spin_lock_bh(>lock);
+
rc = LAPB_OK;
if (lapb->state == LAPB_STATE_1)
goto out_put;
@@ -285,6 +297,7 @@ int lapb_connect_request(struct net_device *dev)
rc = LAPB_OK;
out_put:
+ spin_unlock_bh(>lock);
lapb_put(lapb);
out:
return rc;
@@ -299,6
Re: [PATCH] net: remove disc_data_lock in ppp line discipline
> In tty layer, it use tty->ldisc_sem to proect tty_ldisc_ops. > So I think tty->ldisc_sem can also protect tty->disc_data; It might help by CC'ing TTY people, so that we could get this reviewed by people who are familiar with TTY code. Greg Kroah-Hartman (supporter:TTY LAYER) Jiri Slaby (supporter:TTY LAYER) Thanks! > For examlpe, > When cpu A is running ppp_synctty_ioctl that hold the tty->ldisc_sem, > at the same time if cpu B calls ppp_synctty_close, it will wait until > cpu A release tty->ldisc_sem. So I think it is unnecessary to have the > disc_data_lock; > > cpu A cpu B > tty_ioctl tty_reopen > ->hold tty->ldisc_sem->hold tty->ldisc_sem(write), failed > ->ld->ops->ioctl ->wait... > ->release tty->ldisc_sem ->wait...OK,hold tty->ldisc_sem > ->tty_ldisc_reinit > ->tty_ldisc_close > ->ld->ops->close IMHO an example might not be necessary. Examples are useful to show incorrectness. But we cannot show correctness by examples because examples are not exhaustive. BTW, there're some typos: "proect" -> "protect" "examlpe" -> "example" "that hold ..." -> "that holds ..." "cpu A release ..." -> "cpu A releases ..." > * FIXME: this is no longer true. The _close path for the ldisc is > * now guaranteed to be sane. > */ > * > * FIXME: Fixed in tty_io nowadays. > */ Since you are removing "disc_data_lock", please update (or remove) these two comments. Thanks!
[PATCH net] net: lapb: Decrease the refcount of "struct lapb_cb" in lapb_device_event
In lapb_device_event, lapb_devtostruct is called to get a reference to
an object of "struct lapb_cb". lapb_devtostruct increases the refcount
of the object and returns a pointer to it. However, we didn't decrease
the refcount after we finished using the pointer. This patch fixes this
problem.
Fixes: a4989fa91110 ("net/lapb: support netdev events")
Cc: Martin Schiller
Signed-off-by: Xie He
---
net/lapb/lapb_iface.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/net/lapb/lapb_iface.c b/net/lapb/lapb_iface.c
index 213ea7abc9ab..40961889e9c0 100644
--- a/net/lapb/lapb_iface.c
+++ b/net/lapb/lapb_iface.c
@@ -489,6 +489,7 @@ static int lapb_device_event(struct notifier_block *this,
unsigned long event,
break;
}
+ lapb_put(lapb);
return NOTIFY_DONE;
}
--
2.27.0
Re: [PATCH net v2] net: hdlc_ppp: Fix issues when mod_timer is called while timer is running
On Tue, Dec 29, 2020 at 12:10 AM Hillf Danton wrote: > > >The code path that calls add_timer is for sending keep-alive packets > >when operating in the OPENED state. If we have just changed to the > >OPENED state in ppp_cp_event, we should wait for the amount of time > >set by the (2nd) mod_timer call in ppp_cp_event, before firing the > > What if your change also covers the first case of mod_timer() in > ppp_cp_event()? Yes, for the 1st mod_timer call in ppp_cp_event, the situation is the same. If it is called, it means we are sending out a Configure Request or Terminate Request. In this case, we should wait for the amount of time set by this mod_timer call, before firing the timer. We shouldn't fire the timer immediately because this is not the intention of this mod_timer call. > >timer. We shouldn't fire the timer immediately after we change to the > >OPENED state. This is not the intention of the (2nd) mod_timer call in > >ppp_cp_event. Therefore aborting ppp_timer is the correct solution. > > > Given an expiring timer, is it the right time to call ppp_tx_flush() in > addition to add/mod_timer()? Do you mean when we are aborting ppp_timer, whether we need to call ppp_tx_flush in the aborted ppp_timer? I don't think so. Because when ppp_rx (which directly or indirectly calls ppp_cp_event) releases the lock, it will call ppp_tx_flush. So we don't need to call it again.
Re: [PATCH net v2] net: hdlc_ppp: Fix issues when mod_timer is called while timer is running
On Mon, Dec 28, 2020 at 1:17 AM Hillf Danton wrote: > > On Sun, 27 Dec 2020 18:53:39 -0800 Xie He wrote: > > ppp_cp_event is called directly or indirectly by ppp_rx with "ppp->lock" > > held. It may call mod_timer to add a new timer. However, at the same time > > ppp_timer may be already running and waiting for "ppp->lock". In this > > case, there's no need for ppp_timer to continue running and it can just > > exit. > > Because the timer callback loses the race in acquiring the ppp->lock > does not mean it should abort. I think aborting ppp_timer is the correct solution. When mod_timer is called by ppp_cp_event, which is (directly or indirectly) called by ppp_rx, this means we received something on the line that makes the original timer no longer necessary. If the timer is pending, mod_timer will delete it. If the timer is running and waiting for the lock, I think it should abort. This way it would appear that the timer hasn't fired and is deleted before it fires. > > If we let ppp_timer continue running, it may call add_timer. This causes > > kernel panic because add_timer can't be called with a timer pending. > > Meanwhie we can defuse the peril following add_timer() added in > e022c2f07ae5, say by replacing add_timer() with mod_timer(). The code path that calls add_timer is for sending keep-alive packets when operating in the OPENED state. If we have just changed to the OPENED state in ppp_cp_event, we should wait for the amount of time set by the (2nd) mod_timer call in ppp_cp_event, before firing the timer. We shouldn't fire the timer immediately after we change to the OPENED state. This is not the intention of the (2nd) mod_timer call in ppp_cp_event. Therefore aborting ppp_timer is the correct solution.
[PATCH net v2] net: hdlc_ppp: Fix issues when mod_timer is called while timer is running
ppp_cp_event is called directly or indirectly by ppp_rx with "ppp->lock"
held. It may call mod_timer to add a new timer. However, at the same time
ppp_timer may be already running and waiting for "ppp->lock". In this
case, there's no need for ppp_timer to continue running and it can just
exit.
If we let ppp_timer continue running, it may call add_timer. This causes
kernel panic because add_timer can't be called with a timer pending.
This patch fixes this problem.
Fixes: e022c2f07ae5 ("WAN: new synchronous PPP implementation for generic
HDLC.")
Cc: Krzysztof Halasa
Signed-off-by: Xie He
---
drivers/net/wan/hdlc_ppp.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/drivers/net/wan/hdlc_ppp.c b/drivers/net/wan/hdlc_ppp.c
index 64f855651336..261b53fc8e04 100644
--- a/drivers/net/wan/hdlc_ppp.c
+++ b/drivers/net/wan/hdlc_ppp.c
@@ -569,6 +569,13 @@ static void ppp_timer(struct timer_list *t)
unsigned long flags;
spin_lock_irqsave(>lock, flags);
+ /* mod_timer could be called after we entered this function but
+* before we got the lock.
+*/
+ if (timer_pending(>timer)) {
+ spin_unlock_irqrestore(>lock, flags);
+ return;
+ }
switch (proto->state) {
case STOPPING:
case REQ_SENT:
--
2.27.0
[PATCH net] net: hdlc_ppp: Fix issues when mod_timer is called while timer is running
ppp_cp_event is called directly or indirectly by ppp_rx with "ppp->lock"
held. It may call mod_timer to add a new timer. However, at the same time
ppp_timer may be already running and waiting for "ppp->lock". In this
case, there's no need for ppp_timer to continue running and it can just
exit.
If we let ppp_timer continue running, it may call add_timer. This causes
kernel panic because add_timer can't be called with a timer pending.
This patch fixes this problem.
Cc: Krzysztof Halasa
Signed-off-by: Xie He
---
drivers/net/wan/hdlc_ppp.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/drivers/net/wan/hdlc_ppp.c b/drivers/net/wan/hdlc_ppp.c
index 64f855651336..261b53fc8e04 100644
--- a/drivers/net/wan/hdlc_ppp.c
+++ b/drivers/net/wan/hdlc_ppp.c
@@ -569,6 +569,13 @@ static void ppp_timer(struct timer_list *t)
unsigned long flags;
spin_lock_irqsave(>lock, flags);
+ /* mod_timer could be called after we entered this function but
+* before we got the lock.
+*/
+ if (timer_pending(>timer)) {
+ spin_unlock_irqrestore(>lock, flags);
+ return;
+ }
switch (proto->state) {
case STOPPING:
case REQ_SENT:
--
2.27.0
Re: [PATCH AUTOSEL 5.4 075/130] net/lapb: fix t1 timer handling for LAPB_STATE_0
On Wed, Dec 23, 2020 at 9:01 AM Xie He wrote: > > I don't think this patch is suitable for stable branches. This patch is > part of a patch series that changes the lapb module from "establishing the > L2 connection only when needed by L3", to "establishing the L2 connection > automatically whenever we are able to". This is a behavioral change. It > should be seen as a new feature. It is not a bug fix. Applying this patch without other patches in the same series will also introduce problems, because this patch relies on part of the changes in the subsequent patch in the same series to be correct. Hi Martin, It's better that we avoid using words like "fix" in non-bug-fix patches, and make every patch work on its own without subsequent patches. Otherwise we'll make people confused.
Re: [PATCH AUTOSEL 5.4 075/130] net/lapb: fix t1 timer handling for LAPB_STATE_0
> From: Martin Schiller > > [ Upstream commit 62480b992ba3fb1d7260b11293aed9d6557831c7 ] > > 1. DTE interface changes immediately to LAPB_STATE_1 and start sending >SABM(E). > > 2. DCE interface sends N2-times DM and changes to LAPB_STATE_1 >afterwards if there is no response in the meantime. I don't think this patch is suitable for stable branches. This patch is part of a patch series that changes the lapb module from "establishing the L2 connection only when needed by L3", to "establishing the L2 connection automatically whenever we are able to". This is a behavioral change. It should be seen as a new feature. It is not a bug fix.
Re: [PATCH net-next] net: x25: Remove unimplemented X.25-over-LLC code stubs
On Thu, Dec 10, 2020 at 1:14 AM David Laight wrote: > > > To me, LLC1 and LLC2 are to Ethernet what UDP and TCP are to IP > > networks. I think we can use LLC1 and LLC2 wherever UDP and TCP can be > > used, as long as we are in the same LAN and are willing to use MAC > > addresses as the addresses. > > Except that you don't have any where near enough 'ports' so you need > something to demultiplex messages to different applications. Yes, LLC only has 256 "ports" compared to more than 6 for UDP/TCP. > We (ICL) always ran class 4 transport (which does error recovery) > directly over LLC1 using MAC address (a NUL byte for the network layer). > This requires a bridged network and globally unique MAC addresses. > Sending out an LLC reflect packet to the broadcast MAC address used to > generate a couple of thousand responses (many would get discarded > because the bridges got overloaded). Wow, You have a really big LAN! > > X.25 layer 3 certainly can also run over LLC2. > > You don't need X.25 layer 3. > X.25 layer 2 does error recovery over a point-to-point link. > X.25 layer 3 does switching between machines. > Class 2 transport does multiplexing over a reliable lower layer. > So you normally need all three. Yes, I was just saying X.25 layer 3 can run over any reliable point-to-point links, including X.25 layer 2, LLC2 and TCP. > However LLC2 gives you a reliable connection between two machines > (selected by MAC address). > So you should be able to run Class 2 transport (well one of its > 4 variants!) directly over LL2. Yes. > The advantage over Class 4 transport over LLC1 is that there is > only one set of retransmit buffers (etc) regardless of the number > of connections. Right. But nowadays we have big enough memories for many buffers, so it may be preferable to make connections operate independent of each other. This way one lost frame wouldn't affect all connections. This is also why HTTP3 moved to QUIC instead of using TCP. > But this is all 30 year old history... Haha, we are talking about really old technologies.
Re: [PATCH net-next] net: x25: Fix handling of Restart Request and Restart Confirmation
On Wed, Dec 9, 2020 at 10:35 PM Martin Schiller wrote: > > Yes, that's also the reason why I already acked this patch. We can > solve this later a little bit cleaner if necessary. > > My patch that takes care of the orphaned packets in x25_receive_data() > has again a dependency on other patches, especially the patch to > configure the neighbor parameters (DCE/DTE, number of channels etc.), > which I already sent before but still have to revise. > > Unfortunately I have only limited time for this topic, so I am not as > fast as some people would wish. Sorry for that. OK. Thanks! I appreciate your work! Code needs to have specialist developers like you to keep it alive and evolving. I understand you have limited time. Please take your time. Thanks!
Re: [PATCH net-next v7 4/5] net/x25: fix restart request/confirm handling
On Wed, Dec 9, 2020 at 10:27 PM Martin Schiller wrote: > > > Hi Martin, > > > > When you submit future patch series, can you try ensuring the code to > > be in a completely working state after each patch in the series? This > > makes reviewing the patches easier. After the patches get applied, > > this also makes tracing bugs (for example, with "git bisect") through > > the commit history easier. > > Well I thought that's what patch series are for: > Send patches that belong together and should be applied together. > > Of course I will try to make each patch work on its own, but this is not > always possible with major changes or ends up in monster patches. > And nobody wants that. Thanks! I admit that this series is a big change and is not easy to split up properly. If I were you, I may end up sending a very big patch first, and then follow up with small patches for "restart request/confirm handling" and "add/remove x25_neigh on device-register/unregister instead of device-up/down". (The little change in x25_link_established should belong to the first big patch instead of "restart request/confirm handling".) But making each patch work on its own is indeed preferable. I see https://www.kernel.org/doc/html/latest/process/submitting-patches.html says: When dividing your change into a series of patches, take special care to ensure that the kernel builds and runs properly after each patch in the series. Developers using git bisect to track down a problem can end up splitting your patch series at any point; they will not thank you if you introduce bugs in the middle.
Re: [PATCH net-next] net: x25: Remove unimplemented X.25-over-LLC code stubs
On Wed, Dec 9, 2020 at 1:21 PM David Laight wrote: > > I always wondered about running Class 2 transport directly over LLC2 > (rather than Class 4 over LLC1). > But the only LLC2 user was netbios - and microsoft's LLC2 was broken. > Not to mention the window probing needed to handle systems that > said they supported a window of (IIRC) 15 but would discard the > 5th back to back frame. To me, LLC1 and LLC2 are to Ethernet what UDP and TCP are to IP networks. I think we can use LLC1 and LLC2 wherever UDP and TCP can be used, as long as we are in the same LAN and are willing to use MAC addresses as the addresses. X.25 layer 3 certainly can also run over LLC2. Linux actually has support for LLC1 and LLC2. User space programs can transmit data directly over LLC1 and LLC2 using "AF_LLC" sockets.
Re: [PATCH net-next v7 4/5] net/x25: fix restart request/confirm handling
On Wed, Dec 9, 2020 at 1:47 AM Xie He wrote: > > On Wed, Dec 9, 2020 at 1:41 AM Martin Schiller wrote: > > > > Right. > > By the way: A "Restart Collision" is in practice a very common event to > > establish the Layer 3. > > Oh, I see. Thanks! Hi Martin, When you submit future patch series, can you try ensuring the code to be in a completely working state after each patch in the series? This makes reviewing the patches easier. After the patches get applied, this also makes tracing bugs (for example, with "git bisect") through the commit history easier.
Re: [PATCH net-next] net: x25: Fix handling of Restart Request and Restart Confirmation
On Wed, Dec 9, 2020 at 2:31 AM Martin Schiller wrote: > > >> 1. When the x25 module gets loaded, layer 2 may already be running and > >> connected. In this case, although we are in X25_LINK_STATE_0, we still > >> need to handle the Restart Request received, rather than ignore it. > > > > Hmm... I've never loaded the X.25 module after the interface is UP, but > > in this case we really have to fix it. > > > > This seems to be a regression caused by moving the Layer2 link handling > into the lapb driver, which wasn't intended in my original patchset. > > I also have another patch on my todo list which aims orphan packet > handling in the x25_receive_data() function. Maybe it is better to catch > the whole thing there. OK.. Currently it's not clear to me what your future patches would be. Maybe we can first have this patch applied? Because based on the current code I think this patch is necessary. When you are ready to submit your patches, you can replace my code and we can discuss further.
Re: [PATCH net-next v7 4/5] net/x25: fix restart request/confirm handling
On Wed, Dec 9, 2020 at 1:41 AM Martin Schiller wrote: > > Right. > By the way: A "Restart Collision" is in practice a very common event to > establish the Layer 3. Oh, I see. Thanks!
Re: [PATCH net-next v7 4/5] net/x25: fix restart request/confirm handling
On Wed, Dec 9, 2020 at 1:01 AM Xie He wrote:
>
> On Wed, Nov 25, 2020 at 10:36 PM Martin Schiller wrote:
> >
> > switch (nb->state) {
> > case X25_LINK_STATE_0:
> > - nb->state = X25_LINK_STATE_2;
> > - break;
> > case X25_LINK_STATE_1:
> > x25_transmit_restart_request(nb);
> > nb->state = X25_LINK_STATE_2;
>
> What is the reason for this change? Originally only the connecting
> side will transmit a Restart Request; the connected side will not and
> will only wait for the Restart Request to come. Now both sides will
> transmit Restart Requests at the same time. I think we should better
> avoid collision situations like this.
Oh. I see. Because in other patches we are giving L2 the ability to
connect by itself, both sides can now appear here to be the
"connected" side. So we can't make the "connected" side wait as we did
before.
Re: [PATCH net-next v7 4/5] net/x25: fix restart request/confirm handling
On Wed, Nov 25, 2020 at 10:36 PM Martin Schiller wrote:
>
> We have to take the actual link state into account to handle
> restart requests/confirms well.
>
> @@ -214,8 +241,6 @@ void x25_link_established(struct x25_neigh *nb)
> {
> switch (nb->state) {
> case X25_LINK_STATE_0:
> - nb->state = X25_LINK_STATE_2;
> - break;
> case X25_LINK_STATE_1:
> x25_transmit_restart_request(nb);
> nb->state = X25_LINK_STATE_2;
What is the reason for this change? Originally only the connecting
side will transmit a Restart Request; the connected side will not and
will only wait for the Restart Request to come. Now both sides will
transmit Restart Requests at the same time. I think we should better
avoid collision situations like this.
[PATCH net-next] net: x25: Fix handling of Restart Request and Restart Confirmation
1. When the x25 module gets loaded, layer 2 may already be running and
connected. In this case, although we are in X25_LINK_STATE_0, we still
need to handle the Restart Request received, rather than ignore it.
2. When we are in X25_LINK_STATE_2, we have already sent a Restart Request
and is waiting for the Restart Confirmation with t20timer. t20timer will
restart itself repeatedly forever so it will always be there, as long as we
are in State 2. So we don't need to check x25_t20timer_pending again.
Fixes: d023b2b9ccc2 ("net/x25: fix restart request/confirm handling")
Cc: Martin Schiller
Signed-off-by: Xie He
---
net/x25/x25_link.c | 25 +
1 file changed, 9 insertions(+), 16 deletions(-)
diff --git a/net/x25/x25_link.c b/net/x25/x25_link.c
index f92073f3cb11..57a81100c5da 100644
--- a/net/x25/x25_link.c
+++ b/net/x25/x25_link.c
@@ -58,11 +58,6 @@ static inline void x25_stop_t20timer(struct x25_neigh *nb)
del_timer(>t20timer);
}
-static inline int x25_t20timer_pending(struct x25_neigh *nb)
-{
- return timer_pending(>t20timer);
-}
-
/*
* This handles all restart and diagnostic frames.
*/
@@ -70,17 +65,20 @@ void x25_link_control(struct sk_buff *skb, struct x25_neigh
*nb,
unsigned short frametype)
{
struct sk_buff *skbn;
- int confirm;
switch (frametype) {
case X25_RESTART_REQUEST:
switch (nb->state) {
+ case X25_LINK_STATE_0:
+ /* This can happen when the x25 module just gets loaded
+* and doesn't know layer 2 has already connected
+*/
+ nb->state = X25_LINK_STATE_3;
+ x25_transmit_restart_confirmation(nb);
+ break;
case X25_LINK_STATE_2:
- confirm = !x25_t20timer_pending(nb);
x25_stop_t20timer(nb);
nb->state = X25_LINK_STATE_3;
- if (confirm)
- x25_transmit_restart_confirmation(nb);
break;
case X25_LINK_STATE_3:
/* clear existing virtual calls */
@@ -94,13 +92,8 @@ void x25_link_control(struct sk_buff *skb, struct x25_neigh
*nb,
case X25_RESTART_CONFIRMATION:
switch (nb->state) {
case X25_LINK_STATE_2:
- if (x25_t20timer_pending(nb)) {
- x25_stop_t20timer(nb);
- nb->state = X25_LINK_STATE_3;
- } else {
- x25_transmit_restart_request(nb);
- x25_start_t20timer(nb);
- }
+ x25_stop_t20timer(nb);
+ nb->state = X25_LINK_STATE_3;
break;
case X25_LINK_STATE_3:
/* clear existing virtual calls */
--
2.27.0
[PATCH net-next] net: x25: Remove unimplemented X.25-over-LLC code stubs
According to the X.25 documentation, there was a plan to implement
X.25-over-802.2-LLC. It never finished but left various code stubs in the
X.25 code. At this time it is unlikely that it would ever finish so it
may be better to remove those code stubs.
Also change the documentation to make it clear that this is not a ongoing
plan anymore. Change words like "will" to "could", "would", etc.
Cc: Martin Schiller
Signed-off-by: Xie He
---
Documentation/networking/x25.rst | 12 +---
net/x25/af_x25.c | 6 +-
net/x25/x25_dev.c| 13 -
net/x25/x25_route.c | 7 +--
4 files changed, 7 insertions(+), 31 deletions(-)
diff --git a/Documentation/networking/x25.rst b/Documentation/networking/x25.rst
index 00e45d384ba0..e11d9ebdf9a3 100644
--- a/Documentation/networking/x25.rst
+++ b/Documentation/networking/x25.rst
@@ -19,13 +19,11 @@ implementation of LAPB. Therefore the LAPB modules would be
called by
unintelligent X.25 card drivers and not by intelligent ones, this would
provide a uniform device driver interface, and simplify configuration.
-To confuse matters a little, an 802.2 LLC implementation for Linux is being
-written which will allow X.25 to be run over an Ethernet (or Token Ring) and
-conform with the JNT "Pink Book", this will have a different interface to
-the Packet Layer but there will be no confusion since the class of device
-being served by the LLC will be completely separate from LAPB. The LLC
-implementation is being done as part of another protocol project (SNA) and
-by a different author.
+To confuse matters a little, an 802.2 LLC implementation is also possible
+which could allow X.25 to be run over an Ethernet (or Token Ring) and
+conform with the JNT "Pink Book", this would have a different interface to
+the Packet Layer but there would be no confusion since the class of device
+being served by the LLC would be completely separate from LAPB.
Just when you thought that it could not become more confusing, another
option appeared, XOT. This allows X.25 Packet Layer frames to operate over
diff --git a/net/x25/af_x25.c b/net/x25/af_x25.c
index d41fffb2507b..ff687b97b2d9 100644
--- a/net/x25/af_x25.c
+++ b/net/x25/af_x25.c
@@ -211,11 +211,7 @@ static int x25_device_event(struct notifier_block *this,
unsigned long event,
if (!net_eq(dev_net(dev), _net))
return NOTIFY_DONE;
- if (dev->type == ARPHRD_X25
-#if IS_ENABLED(CONFIG_LLC)
-|| dev->type == ARPHRD_ETHER
-#endif
-) {
+ if (dev->type == ARPHRD_X25) {
switch (event) {
case NETDEV_REGISTER:
case NETDEV_POST_TYPE_CHANGE:
diff --git a/net/x25/x25_dev.c b/net/x25/x25_dev.c
index 25bf72ee6cad..5259ef8f5242 100644
--- a/net/x25/x25_dev.c
+++ b/net/x25/x25_dev.c
@@ -160,10 +160,6 @@ void x25_establish_link(struct x25_neigh *nb)
*ptr = X25_IFACE_CONNECT;
break;
-#if IS_ENABLED(CONFIG_LLC)
- case ARPHRD_ETHER:
- return;
-#endif
default:
return;
}
@@ -179,10 +175,6 @@ void x25_terminate_link(struct x25_neigh *nb)
struct sk_buff *skb;
unsigned char *ptr;
-#if IS_ENABLED(CONFIG_LLC)
- if (nb->dev->type == ARPHRD_ETHER)
- return;
-#endif
if (nb->dev->type != ARPHRD_X25)
return;
@@ -212,11 +204,6 @@ void x25_send_frame(struct sk_buff *skb, struct x25_neigh
*nb)
*dptr = X25_IFACE_DATA;
break;
-#if IS_ENABLED(CONFIG_LLC)
- case ARPHRD_ETHER:
- kfree_skb(skb);
- return;
-#endif
default:
kfree_skb(skb);
return;
diff --git a/net/x25/x25_route.c b/net/x25/x25_route.c
index ec2a39e9b3e6..9fbe4bb38d94 100644
--- a/net/x25/x25_route.c
+++ b/net/x25/x25_route.c
@@ -124,12 +124,7 @@ struct net_device *x25_dev_get(char *devname)
{
struct net_device *dev = dev_get_by_name(_net, devname);
- if (dev &&
- (!(dev->flags & IFF_UP) || (dev->type != ARPHRD_X25
-#if IS_ENABLED(CONFIG_LLC)
- && dev->type != ARPHRD_ETHER
-#endif
- ))){
+ if (dev && (!(dev->flags & IFF_UP) || dev->type != ARPHRD_X25)) {
dev_put(dev);
dev = NULL;
}
--
2.27.0
[PATCH net-next v3] net: hdlc_x25: Remove unnecessary skb_reset_network_header calls
1. In x25_xmit, skb_reset_network_header is not necessary before we call
lapb_data_request. The lapb module doesn't need skb->network_header.
So there is no need to set skb->network_header before calling
lapb_data_request.
2. In x25_data_indication (called by the lapb module after data have
been received), skb_reset_network_header is not necessary before we
call netif_rx. After we call netif_rx, the code in net/core/dev.c will
call skb_reset_network_header before handing the skb to upper layers
(in __netif_receive_skb_core, called by __netif_receive_skb_one_core,
called by __netif_receive_skb, called by process_backlog). So we don't
need to call skb_reset_network_header by ourselves.
Cc: Martin Schiller
Signed-off-by: Xie He
---
drivers/net/wan/hdlc_x25.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/net/wan/hdlc_x25.c b/drivers/net/wan/hdlc_x25.c
index f52b9fed0593..bb164805804e 100644
--- a/drivers/net/wan/hdlc_x25.c
+++ b/drivers/net/wan/hdlc_x25.c
@@ -77,7 +77,6 @@ static int x25_data_indication(struct net_device *dev, struct
sk_buff *skb)
}
skb_push(skb, 1);
- skb_reset_network_header(skb);
ptr = skb->data;
*ptr = X25_IFACE_DATA;
@@ -118,7 +117,6 @@ static netdev_tx_t x25_xmit(struct sk_buff *skb, struct
net_device *dev)
switch (skb->data[0]) {
case X25_IFACE_DATA:/* Data to be transmitted */
skb_pull(skb, 1);
- skb_reset_network_header(skb);
if ((result = lapb_data_request(dev, skb)) != LAPB_OK)
dev_kfree_skb(skb);
return NETDEV_TX_OK;
--
2.27.0
[PATCH net-next v2] net: hdlc_x25: Remove unnecessary skb_reset_network_header calls
1. In x25_xmit, skb_reset_network_header is not necessary before we call
lapb_data_request. The lapb module doesn't need skb->network_header.
So there is no need to set skb->network_header before calling
lapb_data_request.
2. In x25_data_indication (called by the lapb module after some data
have been received), skb_reset_network_header is not necessary before we
call netif_rx. After we call netif_rx, the code in net/core/dev.c will
call skb_reset_network_header before handing the skb to upper layers
(in __netif_receive_skb_core, called by __netif_receive_skb_one_core,
called by __netif_receive_skb, called by process_backlog). So we don't
need to call skb_reset_network_header by ourselves.
Cc: Martin Schiller
Signed-off-by: Xie He
---
drivers/net/wan/hdlc_x25.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/net/wan/hdlc_x25.c b/drivers/net/wan/hdlc_x25.c
index f52b9fed0593..bb164805804e 100644
--- a/drivers/net/wan/hdlc_x25.c
+++ b/drivers/net/wan/hdlc_x25.c
@@ -77,7 +77,6 @@ static int x25_data_indication(struct net_device *dev, struct
sk_buff *skb)
}
skb_push(skb, 1);
- skb_reset_network_header(skb);
ptr = skb->data;
*ptr = X25_IFACE_DATA;
@@ -118,7 +117,6 @@ static netdev_tx_t x25_xmit(struct sk_buff *skb, struct
net_device *dev)
switch (skb->data[0]) {
case X25_IFACE_DATA:/* Data to be transmitted */
skb_pull(skb, 1);
- skb_reset_network_header(skb);
if ((result = lapb_data_request(dev, skb)) != LAPB_OK)
dev_kfree_skb(skb);
return NETDEV_TX_OK;
--
2.27.0
[PATCH net-next] net: hdlc_x25: Remove unnecessary skb_reset_network_header calls
1. In x25_xmit, skb_reset_network_header is not necessary before we call
lapb_data_request. The lapb module doesn't need skb->network_header.
So there is no need to set skb->network_header before calling
lapb_data_request.
2. In x25_data_indication (called by the lapb module after it has
processed the skb), skb_reset_network_header is not necessary before we
call netif_rx. After we call netif_rx, the code in net/core/dev.c will
call skb_reset_network_header before handing the skb to upper layers
(in __netif_receive_skb_core, called by __netif_receive_skb_one_core,
called by __netif_receive_skb, called by process_backlog). So we don't
need to call skb_reset_network_header by ourselves.
Cc: Martin Schiller
Signed-off-by: Xie He
---
drivers/net/wan/hdlc_x25.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/net/wan/hdlc_x25.c b/drivers/net/wan/hdlc_x25.c
index f52b9fed0593..bb164805804e 100644
--- a/drivers/net/wan/hdlc_x25.c
+++ b/drivers/net/wan/hdlc_x25.c
@@ -77,7 +77,6 @@ static int x25_data_indication(struct net_device *dev, struct
sk_buff *skb)
}
skb_push(skb, 1);
- skb_reset_network_header(skb);
ptr = skb->data;
*ptr = X25_IFACE_DATA;
@@ -118,7 +117,6 @@ static netdev_tx_t x25_xmit(struct sk_buff *skb, struct
net_device *dev)
switch (skb->data[0]) {
case X25_IFACE_DATA:/* Data to be transmitted */
skb_pull(skb, 1);
- skb_reset_network_header(skb);
if ((result = lapb_data_request(dev, skb)) != LAPB_OK)
dev_kfree_skb(skb);
return NETDEV_TX_OK;
--
2.27.0
[PATCH net-next] net: lapbether: Consider it successful if (dis)connecting when already (dis)connected
When the upper layer instruct us to connect (or disconnect), but we have
already connected (or disconnected), consider this operation successful
rather than failed.
This can help the upper layer to correct its record about whether we are
connected or not here in layer 2.
The upper layer may not have the correct information about whether we are
connected or not. This can happen if this driver has already been running
for some time when the "x25" module gets loaded.
Another X.25 driver (hdlc_x25) is already doing this, so we make this
driver do this, too.
Cc: Martin Schiller
Signed-off-by: Xie He
---
drivers/net/wan/lapbether.c | 13 +++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wan/lapbether.c b/drivers/net/wan/lapbether.c
index b6be2454b8bd..605fe555e157 100644
--- a/drivers/net/wan/lapbether.c
+++ b/drivers/net/wan/lapbether.c
@@ -55,6 +55,9 @@ struct lapbethdev {
static LIST_HEAD(lapbeth_devices);
+static void lapbeth_connected(struct net_device *dev, int reason);
+static void lapbeth_disconnected(struct net_device *dev, int reason);
+
/* */
/*
@@ -167,11 +170,17 @@ static netdev_tx_t lapbeth_xmit(struct sk_buff *skb,
case X25_IFACE_DATA:
break;
case X25_IFACE_CONNECT:
- if ((err = lapb_connect_request(dev)) != LAPB_OK)
+ err = lapb_connect_request(dev);
+ if (err == LAPB_CONNECTED)
+ lapbeth_connected(dev, LAPB_OK);
+ else if (err != LAPB_OK)
pr_err("lapb_connect_request error: %d\n", err);
goto drop;
case X25_IFACE_DISCONNECT:
- if ((err = lapb_disconnect_request(dev)) != LAPB_OK)
+ err = lapb_disconnect_request(dev);
+ if (err == LAPB_NOTCONNECTED)
+ lapbeth_disconnected(dev, LAPB_OK);
+ else if (err != LAPB_OK)
pr_err("lapb_disconnect_request err: %d\n", err);
fallthrough;
default:
--
2.27.0
