Re: [PATCH net] net: lapb: Copy the skb before sending a packet
Hello: This patch was applied to netdev/net.git (refs/heads/master): On Sun, 31 Jan 2021 21:57:06 -0800 you wrote: > 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. > > [...] Here is the summary with links: - [net] net: lapb: Copy the skb before sending a packet https://git.kernel.org/netdev/net/c/88c7a9fd9bdd You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html
Re: [PATCH net] net: lapb: Copy the skb before sending a packet
On Mon, 1 Feb 2021 22:25:17 -0800 Xie He wrote: > 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: > [...] > > > > > > 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. You're right. I thought cow_head is a little more clever.
RE: [PATCH net] net: lapb: Copy the skb before sending a packet
From: Xie He > Sent: 01 February 2021 16:15 > > 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. What is the fastest link lapb is actually used on these days? 64k used to be 'fast' - so copying the skb isn't going to have a noticeable effect on system performance. We did once get a 'free' upgrade of our X.25 link from 2400 to 9600. Probably due to the power consumption and rack space needed for the older modem. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
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, 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.
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: lapb: Copy the skb before sending a packet
On 01.02.21 06:57, Xie He wrote:
> 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.
>
This sounds a bit like you want skb_cow_head() ... ?
> 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;
> }
>
Re: [PATCH net] net: lapb: Copy the skb before sending a packet
On 2021-02-01 11:49, Xie He wrote: 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. OK. Acked-by: Martin Schiller
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.
Re: [PATCH net] net: lapb: Copy the skb before sending a packet
On 2021-02-01 06:57, Xie He wrote:
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.
What kind of packages do you mean are corrupted?
ETH_P_X25 or 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.
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)?
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;
}
[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
