| Message ID | 20210201055706.415842-1-xie.he.0141@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [net] net: lapb: Copy the skb before sending a packet | expand |
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 <ms@dev.tdt.de> > Signed-off-by: Xie He <xie.he.0141@gmail.com> > --- > 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(&lapb->write_queue); > > do { > - if ((skbn = skb_clone(skb, GFP_ATOMIC)) == NULL) { > + skbn = skb_copy(skb, GFP_ATOMIC); > + if (!skbn) { > skb_queue_head(&lapb->write_queue, skb); > break; > }
On Mon, Feb 1, 2021 at 2:05 AM Martin Schiller <ms@dev.tdt.de> 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.
On 2021-02-01 11:49, Xie He wrote: > On Mon, Feb 1, 2021 at 2:05 AM Martin Schiller <ms@dev.tdt.de> 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 <ms@dev.tdt.de>
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 <ms@dev.tdt.de> > Signed-off-by: Xie He <xie.he.0141@gmail.com> > --- > 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(&lapb->write_queue); > > do { > - if ((skbn = skb_clone(skb, GFP_ATOMIC)) == NULL) { > + skbn = skb_copy(skb, GFP_ATOMIC); > + if (!skbn) { > skb_queue_head(&lapb->write_queue, skb); > break; > } >
On Mon, 1 Feb 2021 08:14:31 -0800 Xie He wrote: > On Mon, Feb 1, 2021 at 6:10 AM Julian Wiedmann <jwi@linux.ibm.com> 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.
On Mon, Feb 1, 2021 at 8:42 PM Jakub Kicinski <kuba@kernel.org> wrote: > > On Mon, 1 Feb 2021 08:14:31 -0800 Xie He wrote: > > On Mon, Feb 1, 2021 at 6:10 AM Julian Wiedmann <jwi@linux.ibm.com> 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.
From: Xie He > Sent: 01 February 2021 16:15 > > On Mon, Feb 1, 2021 at 6:10 AM Julian Wiedmann <jwi@linux.ibm.com> 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)
On Mon, 1 Feb 2021 22:25:17 -0800 Xie He wrote: > On Mon, Feb 1, 2021 at 8:42 PM Jakub Kicinski <kuba@kernel.org> wrote: > > > > On Mon, 1 Feb 2021 08:14:31 -0800 Xie He wrote: > > > On Mon, Feb 1, 2021 at 6:10 AM Julian Wiedmann <jwi@linux.ibm.com> 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.
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
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(&lapb->write_queue); do { - if ((skbn = skb_clone(skb, GFP_ATOMIC)) == NULL) { + skbn = skb_copy(skb, GFP_ATOMIC); + if (!skbn) { skb_queue_head(&lapb->write_queue, skb); break; }
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 <ms@dev.tdt.de> Signed-off-by: Xie He <xie.he.0141@gmail.com> --- net/lapb/lapb_out.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)