[PATCH net] net: lapb: Copy the skb before sending a packet

[PATCH net] net: lapb: Copy the skb before sending a packet

[Xie He]

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(-)

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;
 			}
-- 
2.27.0

Re: [PATCH net] net: lapb: Copy the skb before sending a packet

[Martin Schiller]

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;
>  			}

Re: [PATCH net] net: lapb: Copy the skb before sending a packet

[Xie He]

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.

Re: [PATCH net] net: lapb: Copy the skb before sending a packet

[Martin Schiller]

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>

Re: [PATCH net] net: lapb: Copy the skb before sending a packet

[Julian Wiedmann]

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;
>  			}
> 

Re: [PATCH net] net: lapb: Copy the skb before sending a packet

[Xie He]

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.

Re: [PATCH net] net: lapb: Copy the skb before sending a packet

[Jakub Kicinski]

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.

Re: [PATCH net] net: lapb: Copy the skb before sending a packet

[Xie He]

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.

RE: [PATCH net] net: lapb: Copy the skb before sending a packet

[David Laight]

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)

Re: [PATCH net] net: lapb: Copy the skb before sending a packet

[Jakub Kicinski]

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.

Re: [PATCH net] net: lapb: Copy the skb before sending a packet

[patchwork-bot+netdevbpf]

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