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 <[email protected]>
Signed-off-by: Xie He <[email protected]>
---
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
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 <[email protected]>
> Signed-off-by: Xie He <[email protected]>
> ---
> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]>
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 <[email protected]>
> Signed-off-by: Xie He <[email protected]>
> ---
> 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 6:10 AM Julian Wiedmann <[email protected]> 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.
On Mon, 1 Feb 2021 08:14:31 -0800 Xie He wrote:
> On Mon, Feb 1, 2021 at 6:10 AM Julian Wiedmann <[email protected]> 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 <[email protected]> wrote:
>
> On Mon, 1 Feb 2021 08:14:31 -0800 Xie He wrote:
> > On Mon, Feb 1, 2021 at 6:10 AM Julian Wiedmann <[email protected]> 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 <[email protected]> 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 <[email protected]> wrote:
> >
> > On Mon, 1 Feb 2021 08:14:31 -0800 Xie He wrote:
> > > On Mon, Feb 1, 2021 at 6:10 AM Julian Wiedmann <[email protected]> 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