kernel BUG at net/core/skbuff.c:LINE! (2)

[syzbot]

Hello,

syzkaller hit the following crash on
c69fe407803d4b554b7397fad9598a04717ac255
git://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git/master
compiler: gcc (GCC) 7.1.1 20170620
.config is attached
Raw console output is attached.




sctp: [Deprecated]: syz-executor0 (pid 12122) Use of struct
sctp_assoc_value in delayed_ack socket option.
Use struct sctp_sack_info instead
skbuff: skb_over_panic: text:ffffffff848208a3 len:213348 put:213008
head:ffff8801c99c2140 data:ffff8801c99c21f8 tail:0x3421c end:0x7ec0
dev:<NULL>
------------[ cut here ]------------
kernel BUG at net/core/skbuff.c:105!
invalid opcode: 0000 [#1] SMP KASAN
Dumping ftrace buffer:
(ftrace buffer empty)
Modules linked in:
CPU: 1 PID: 12167 Comm: syz-executor5 Not tainted 4.14.0-rc5+ #100
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
task: ffff8801cd7ba3c0 task.stack: ffff8801c1fc0000
RIP: 0010:skb_panic+0x15c/0x1f0 net/core/skbuff.c:101
RSP: 0018:ffff8801c1fc63a8 EFLAGS: 00010286
RAX: 0000000000000092 RBX: ffff8801a91dc680 RCX: 0000000000000000
RDX: 0000000000000092 RSI: ffffffff8158d77e RDI: ffffed00383f8c69
RBP: ffff8801c1fc6410 R08: 0000000000000000 R09: 1ffff100383f8c17
R10: 000000006d0a2354 R11: ffffffff85b2cc98 R12: ffffffff853bcca0
R13: ffffffff848208a3 R14: 0000000000034010 R15: ffffffff853bc4e0
FS: 00007fbe84337700(0000) GS:ffff8801db300000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000020001f68 CR3: 00000001c9045000 CR4: 00000000001406e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
skb_over_panic net/core/skbuff.c:110 [inline]
skb_put+0x181/0x1c0 net/core/skbuff.c:1699
skb_put_data include/linux/skbuff.h:2047 [inline]
sctp_packet_pack net/sctp/output.c:472 [inline]
sctp_packet_transmit+0x1183/0x3750 net/sctp/output.c:605
sctp_outq_flush+0x1216/0x4050 net/sctp/outqueue.c:1191
sctp_outq_uncork+0x5a/0x70 net/sctp/outqueue.c:772
sctp_cmd_interpreter net/sctp/sm_sideeffect.c:1822 [inline]
sctp_side_effects net/sctp/sm_sideeffect.c:1222 [inline]
sctp_do_sm+0x50e/0x6a30 net/sctp/sm_sideeffect.c:1193
sctp_assoc_bh_rcv+0x283/0x4b0 net/sctp/associola.c:1065
sctp_inq_push+0x23b/0x300 net/sctp/inqueue.c:95
sctp_backlog_rcv+0x177/0xaa0 net/sctp/input.c:350
sk_backlog_rcv include/net/sock.h:912 [inline]
__release_sock+0x124/0x360 net/core/sock.c:2266
release_sock+0xa4/0x2a0 net/core/sock.c:2778
sctp_wait_for_connect+0x346/0x570 net/sctp/socket.c:8099
sctp_sendmsg+0x29fd/0x32b0 net/sctp/socket.c:2009
inet_sendmsg+0x11f/0x5e0 net/ipv4/af_inet.c:763
sock_sendmsg_nosec net/socket.c:633 [inline]
sock_sendmsg+0xca/0x110 net/socket.c:643
SYSC_sendto+0x352/0x5a0 net/socket.c:1750
SyS_sendto+0x40/0x50 net/socket.c:1718
entry_SYSCALL_64_fastpath+0x1f/0xbe
RIP: 0033:0x452869
RSP: 002b:00007fbe84336be8 EFLAGS: 00000212 ORIG_RAX: 000000000000002c
RAX: ffffffffffffffda RBX: 0000000000758020 RCX: 0000000000452869
RDX: 0000000000034000 RSI: 0000000020c9bfff RDI: 0000000000000013
RBP: 0000000000000000 R08: 0000000020a46000 R09: 000000000000001c
R10: 0000000000000000 R11: 0000000000000212 R12: 0000000000000000
R13: 0000000000a6f7ff R14: 00007fbe843379c0 R15: 0000000000000000
Code: 03 0f b6 04 01 84 c0 74 04 3c 03 7e 20 8b 4b 78 41 57 48 c7 c7 20 c5
3b 85 52 56 4c 89 ea 41 50 4c 89 e6 45 89 f0 e8 b9 19 77 fd <0f> 0b 4c 89
4d b8 4c 89 45 c0 48 89 75 c8 48 89 55 d0 e8 6d 4b
RIP: skb_panic+0x15c/0x1f0 net/core/skbuff.c:101 RSP: ffff8801c1fc63a8
---[ end trace 6beb4fe15730f020 ]---


---
This bug is generated by a dumb bot. It may contain errors.
See https://goo.gl/tpsmEJ for details.
Direct all questions to syzk...@googlegroups.com.

syzbot will keep track of this bug report.
Once a fix for this bug is committed, please reply to this email with:
#syz fix: exact-commit-title
To mark this as a duplicate of another syzbot report, please reply with:
#syz dup: exact-subject-of-another-report
If it's a one-off invalid bug report, please reply with:
#syz invalid
Note: if the crash happens again, it will cause creation of a new bug
report.

[Xin Long]

On Mon, Oct 30, 2017 at 10:36 PM, syzbot
<bot+ed0838d0fa4c4f2b52...@syzkaller.appspotmail.com>
wrote:

> Hello,
>
> syzkaller hit the following crash on
> c69fe407803d4b554b7397fad9598a04717ac255
> git://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git/master
> compiler: gcc (GCC) 7.1.1 20170620
> .config is attached
> Raw console output is attached.
>
>
>
>
> sctp: [Deprecated]: syz-executor0 (pid 12122) Use of struct sctp_assoc_value
> in delayed_ack socket option.
> Use struct sctp_sack_info instead
> skbuff: skb_over_panic: text:ffffffff848208a3 len:213348 put:213008
> head:ffff8801c99c2140 data:ffff8801c99c21f8 tail:0x3421c end:0x7ec0
> dev:<NULL>

'put:213008' is buggy. we need to check strreset_req chunk, that's the
only chunk I could see may exceed SCTP_MAX_CHUNK_LEN.

--- a/net/sctp/sm_make_chunk.c
+++ b/net/sctp/sm_make_chunk.c
@@ -3603,6 +3603,9 @@ struct sctp_chunk *sctp_make_strreset_req(
outlen = (sizeof(outreq) + stream_len) * out;
inlen = (sizeof(inreq) + stream_len) * in;

+ if (outlen + inlen > SCTP_MAX_CHUNK_LEN - sizeof(struct sctp_chunkhdr))
+ return NULL;
+

[syzbot]

syzkaller has found reproducer for the following crash on
82bcf1def3b5f1251177ad47c44f7e17af039b4b
git://git.cmpxchg.org/linux-mmots.git/master

compiler: gcc (GCC) 7.1.1 20170620
.config is attached
Raw console output is attached.

syzkaller reproducer is attached. See https://goo.gl/kgGztJ
for information about syzkaller reproducers


skbuff: skb_over_panic: text:0000000010b86b8d len:196 put:20
head:000000003b477e60 data:000000000e85441e tail:0xd4 end:0xc0 dev:lo
------------[ cut here ]------------
kernel BUG at net/core/skbuff.c:104!

invalid opcode: 0000 [#1] SMP KASAN
Dumping ftrace buffer:
(ftrace buffer empty)
Modules linked in:

CPU: 1 PID: 0 Comm: swapper/1 Not tainted 4.15.0-rc2-mm1+ #39

Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011

RIP: 0010:skb_panic+0x15c/0x1f0 net/core/skbuff.c:100
RSP: 0018:ffff8801db307508 EFLAGS: 00010286
RAX: 0000000000000082 RBX: ffff8801c517e840 RCX: 0000000000000000
RDX: 0000000000000082 RSI: 1ffff1003b660e61 RDI: ffffed003b660e95
RBP: ffff8801db307570 R08: 1ffff1003b660e23 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000000 R12: ffffffff85bd4020
R13: ffffffff84754ed2 R14: 0000000000000014 R15: ffff8801c4e26540
FS: 0000000000000000(0000) GS:ffff8801db300000(0000) knlGS:0000000000000000

CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033

CR2: 0000000000463610 CR3: 00000001c6698000 CR4: 00000000001406e0

DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:

<IRQ>
skb_over_panic net/core/skbuff.c:109 [inline]
skb_put+0x181/0x1c0 net/core/skbuff.c:1694
add_grhead.isra.24+0x42/0x3b0 net/ipv6/mcast.c:1695
add_grec+0xa55/0x1060 net/ipv6/mcast.c:1817
mld_send_cr net/ipv6/mcast.c:1903 [inline]
mld_ifc_timer_expire+0x4d2/0x770 net/ipv6/mcast.c:2448
call_timer_fn+0x23b/0x840 kernel/time/timer.c:1320
expire_timers kernel/time/timer.c:1357 [inline]
__run_timers+0x7e1/0xb60 kernel/time/timer.c:1660
run_timer_softirq+0x4c/0xb0 kernel/time/timer.c:1686
__do_softirq+0x29d/0xbb2 kernel/softirq.c:285
invoke_softirq kernel/softirq.c:365 [inline]
irq_exit+0x1d3/0x210 kernel/softirq.c:405
exiting_irq arch/x86/include/asm/apic.h:540 [inline]
smp_apic_timer_interrupt+0x16b/0x700 arch/x86/kernel/apic/apic.c:1052
apic_timer_interrupt+0xa9/0xb0 arch/x86/entry/entry_64.S:920
</IRQ>
RIP: 0010:native_safe_halt+0x6/0x10 arch/x86/include/asm/irqflags.h:54
RSP: 0018:ffff8801d9f97da8 EFLAGS: 00000282 ORIG_RAX: ffffffffffffff11
RAX: dffffc0000000000 RBX: 1ffff1003b3f2fb8 RCX: 0000000000000000
RDX: 1ffffffff0c59734 RSI: 0000000000000001 RDI: ffffffff862cb9a0
RBP: ffff8801d9f97da8 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000001
R13: ffff8801d9f97e60 R14: ffffffff869eb920 R15: 0000000000000000
arch_safe_halt arch/x86/include/asm/paravirt.h:93 [inline]
default_idle+0xbf/0x430 arch/x86/kernel/process.c:355
arch_cpu_idle+0xa/0x10 arch/x86/kernel/process.c:346
default_idle_call+0x36/0x90 kernel/sched/idle.c:98
cpuidle_idle_call kernel/sched/idle.c:156 [inline]
do_idle+0x24a/0x3b0 kernel/sched/idle.c:246
cpu_startup_entry+0x18/0x20 kernel/sched/idle.c:351
start_secondary+0x330/0x460 arch/x86/kernel/smpboot.c:277
secondary_startup_64+0xa5/0xb0 arch/x86/kernel/head_64.S:237
Code: 03 0f b6 04 01 84 c0 74 04 3c 03 7e 20 8b 4b 78 41 57 48 c7 c7 a0 38
bd 85 52 56 4c 89 ea 41 50 4c 89 e6 45 89 f0 e8 0c b6 3d fd <0f> 0b 4c 89
4d b8 4c 89 45 c0 48 89 75 c8 48 89 55 d0 e8 7d 93
RIP: skb_panic+0x15c/0x1f0 net/core/skbuff.c:100 RSP: ffff8801db307508
---[ end trace 941a8a0f633e271f ]---

[Xin Long]

On Fri, Dec 8, 2017 at 4:16 PM, syzbot
<bot+ed0838d0fa4c4f2b52...@syzkaller.appspotmail.com>
wrote:

This isn't a sctp problem, but mld's, seems when lo's mtu became 0,
it allocs a skb without enough space in add_grec():
if (AVAILABLE(skb) < sizeof(*psrc) +
first*sizeof(struct mld2_grec)) {
if (truncate && !first)
break; /* truncate these */
if (pgr)
pgr->grec_nsrcs = htons(scount);
if (skb)
mld_sendpack(skb);
skb = mld_newpack(idev, dev->mtu); <---

I will check this for sure later on both igmp and mld.

[Xin Long]

Fix:
--- a/net/ipv6/mcast.c
+++ b/net/ipv6/mcast.c
@@ -1766,8 +1766,8 @@ static struct sk_buff *add_grec(struct sk_buff
*skb, struct ifmcaddr6 *pmc,
if (isquery)
psf->sf_gsresp = 0;

- if (AVAILABLE(skb) < sizeof(*psrc) +
- first*sizeof(struct mld2_grec)) {
+ if (AVAILABLE(skb) < (int)(sizeof(*psrc) +
+ first * sizeof(*pgr))) {

if (truncate && !first)
break; /* truncate these */
if (pgr)

@@ -1810,7 +1810,7 @@ static struct sk_buff *add_grec(struct sk_buff
*skb, struct ifmcaddr6 *pmc,
return skb;
if (pmc->mca_crcount || isquery || crsend) {
/* make sure we have room for group header */
- if (skb && AVAILABLE(skb) < sizeof(struct mld2_grec)) {
+ if (skb && AVAILABLE(skb) < (int)sizeof(*pgr)) {
mld_sendpack(skb);
skb = NULL; /* add_grhead will get a new one */
}

do the same on igmp.

[Eric Dumazet]

Thanks for the tentative patch.

Quite a hack if you ask me.

I would rather :

1) Read dev->mtu once to avoid bad assumptions/surprises.

2) Give up if this mtu is too small for IPV6 to be functional.

Something like :


net/ipv6/mcast.c | 25 +++++++++++++++----------
1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/net/ipv6/mcast.c b/net/ipv6/mcast.c
index fc6d7d143f2c29aab9a3f56eae02e5337e65a97b..844642682b8363c4c32d329ed92474f834a59618 100644
--- a/net/ipv6/mcast.c
+++ b/net/ipv6/mcast.c
@@ -1682,16 +1682,16 @@ static int grec_size(struct ifmcaddr6 *pmc, int type, int gdel, int sdel)
}

static struct sk_buff *add_grhead(struct sk_buff *skb, struct ifmcaddr6 *pmc,
- int type, struct mld2_grec **ppgr)
+ int type, struct mld2_grec **ppgr, unsigned int mtu)
{
- struct net_device *dev = pmc->idev->dev;
struct mld2_report *pmr;
struct mld2_grec *pgr;

- if (!skb)
- skb = mld_newpack(pmc->idev, dev->mtu);
- if (!skb)
- return NULL;
+ if (!skb) {
+ skb = mld_newpack(pmc->idev, mtu);
+ if (!skb)
+ return NULL;
+ }
pgr = skb_put(skb, sizeof(struct mld2_grec));
pgr->grec_type = type;
pgr->grec_auxwords = 0;
@@ -1714,10 +1714,15 @@ static struct sk_buff *add_grec(struct sk_buff *skb, struct ifmcaddr6 *pmc,
struct mld2_grec *pgr = NULL;
struct ip6_sf_list *psf, *psf_next, *psf_prev, **psf_list;
int scount, stotal, first, isquery, truncate;
+ unsigned int mtu;

if (pmc->mca_flags & MAF_NOREPORT)
return skb;

+ mtu = READ_ONCE(dev->mtu);
+ if (mtu < IPV6_MIN_MTU)
+ return skb;
+
isquery = type == MLD2_MODE_IS_INCLUDE ||
type == MLD2_MODE_IS_EXCLUDE;
truncate = type == MLD2_MODE_IS_EXCLUDE ||
@@ -1738,7 +1743,7 @@ static struct sk_buff *add_grec(struct sk_buff *skb, struct ifmcaddr6 *pmc,
AVAILABLE(skb) < grec_size(pmc, type, gdeleted, sdeleted)) {
if (skb)
mld_sendpack(skb);
- skb = mld_newpack(idev, dev->mtu);
+ skb = mld_newpack(idev, mtu);
}
}
first = 1;
@@ -1774,12 +1779,12 @@ static struct sk_buff *add_grec(struct sk_buff *skb, struct ifmcaddr6 *pmc,

pgr->grec_nsrcs = htons(scount);
if (skb)
mld_sendpack(skb);

- skb = mld_newpack(idev, dev->mtu);
+ skb = mld_newpack(idev, mtu);
first = 1;
scount = 0;
}
if (first) {
- skb = add_grhead(skb, pmc, type, &pgr);
+ skb = add_grhead(skb, pmc, type, &pgr, mtu);
first = 0;
}
if (!skb)
@@ -1814,7 +1819,7 @@ static struct sk_buff *add_grec(struct sk_buff *skb, struct ifmcaddr6 *pmc,

mld_sendpack(skb);
skb = NULL; /* add_grhead will get a new one */
}

- skb = add_grhead(skb, pmc, type, &pgr);
+ skb = add_grhead(skb, pmc, type, &pgr, mtu);
}
}
if (pgr)

[Cong Wang]

On Fri, Dec 8, 2017 at 12:45 AM, Xin Long <lucie...@gmail.com> wrote:
> This isn't a sctp problem, but mld's, seems when lo's mtu became 0,
> it allocs a skb without enough space in add_grec():

Shouldn't we just set its min_mtu to ETH_MIN_MTU?

[Xin Long]

The new patch works to me, just two questions:
1. should it use "idev->cnf.mtu6" here for mld ?

2. 'if (int < unsigned int)' is still not nice, though in 'if
(AVAILABLE(skb) < sizeof())'
AVAILABLE(skb) seems always to return >= 0 after your patch.

[Xin Long]

No idea why there's no min_mtu limitation for lo dev.

[Eric Dumazet]

On Sun, 2017-12-10 at 12:36 +0800, Xin Long wrote:
> The new patch works to me, just two questions:
> 1. should it use "idev->cnf.mtu6" here for mld ?

No idea why some parts of IPv6 would use a different view of device
mtu. Maybe others can comment.

>
> 2.  'if (int < unsigned int)' is still not nice, though in 'if
> (AVAILABLE(skb) < sizeof())'
>      AVAILABLE(skb) seems always to return >= 0 after your patch.

Really if AVAILABLE(skb) was negative, a bug already happened.

[Eric Dumazet]

Because it wont solve this bug.

ETH_MIN_MTU is really about IPv4.

[syzbot]

syzkaller has found reproducer for the following crash on

b625c1ff82272e26c76570d3c7123419ec345b20
git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/master

compiler: gcc (GCC) 7.1.1 20170620
.config is attached
Raw console output is attached.

C reproducer is attached

syzkaller reproducer is attached. See https://goo.gl/kgGztJ
for information about syzkaller reproducers

IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by:
syzbot+ed0838d0fa4c4f2b...@syzkaller.appspotmail.com
It will help syzbot understand when the bug is fixed.

skbuff: skb_under_panic: text:000000001d390b3a len:31 put:24
head:00000000d8ed776f data:000000008150e823 tail:0x7 end:0xc0 dev:gre0
------------[ cut here ]------------
kernel BUG at net/core/skbuff.c:104!

invalid opcode: 0000 [#1] SMP KASAN
Dumping ftrace buffer:
(ftrace buffer empty)
Modules linked in:

CPU: 1 PID: 3670 Comm: syzkaller801466 Not tainted
4.15.0-rc7-next-20180115+ #97

Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011

RIP: 0010:skb_panic+0x162/0x1f0 net/core/skbuff.c:100
RSP: 0018:ffff8801d9bd7840 EFLAGS: 00010282
RAX: 0000000000000083 RBX: ffff8801d4f083c0 RCX: 0000000000000000
RDX: 0000000000000083 RSI: 1ffff1003b37ae92 RDI: ffffed003b37aefc
RBP: ffff8801d9bd78a8 R08: 1ffff1003b37ae8a R09: 0000000000000000
R10: 0000000000000001 R11: 0000000000000000 R12: ffffffff86200de0
R13: ffffffff84a981ad R14: 0000000000000018 R15: ffff8801d2d34180
FS: 00000000019c4880(0000) GS:ffff8801db300000(0000) knlGS:0000000000000000

CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033

CR2: 00000000208bc000 CR3: 00000001d9111001 CR4: 00000000001606e0

DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:

skb_under_panic net/core/skbuff.c:114 [inline]
skb_push+0xce/0xf0 net/core/skbuff.c:1714
ipgre_header+0x6d/0x4e0 net/ipv4/ip_gre.c:879
dev_hard_header include/linux/netdevice.h:2723 [inline]
pppoe_sendmsg+0x58e/0x8b0 drivers/net/ppp/pppoe.c:890
sock_sendmsg_nosec net/socket.c:630 [inline]
sock_sendmsg+0xca/0x110 net/socket.c:640
sock_write_iter+0x31a/0x5d0 net/socket.c:909
call_write_iter include/linux/fs.h:1775 [inline]
do_iter_readv_writev+0x525/0x7f0 fs/read_write.c:653
do_iter_write+0x154/0x540 fs/read_write.c:932
vfs_writev+0x18a/0x340 fs/read_write.c:977
do_writev+0xfc/0x2a0 fs/read_write.c:1012
SYSC_writev fs/read_write.c:1085 [inline]
SyS_writev+0x27/0x30 fs/read_write.c:1082
entry_SYSCALL_64_fastpath+0x29/0xa0
RIP: 0033:0x445009
RSP: 002b:00007ffcab0aa648 EFLAGS: 00000217 ORIG_RAX: 0000000000000014
RAX: ffffffffffffffda RBX: ffffffffffffffff RCX: 0000000000445009
RDX: 0000000000000001 RSI: 0000000020211f90 RDI: 0000000000000004
RBP: 00007ffcab0aa748 R08: 0000000020adffb2 R09: 0000000020adffb2
R10: 0000000020adffb2 R11: 0000000000000217 R12: 00007ffcab0aa748
R13: 0000000000402510 R14: 0000000000000000 R15: 0000000000000000
Code: 04 01 84 c0 74 04 3c 03 7e 23 8b 8b 80 00 00 00 41 57 48 c7 c7 a0 06
20 86 52 56 4c 89 ea 41 50 4c 89 e6 45 89 f0 e8 b6 c9 23 fd <0f> 0b 4c 89
4d b8 4c 89 45 c0 48 89 75 c8 48 89 55 d0 e8 37 42
RIP: skb_panic+0x162/0x1f0 net/core/skbuff.c:100 RSP: ffff8801d9bd7840
---[ end trace 1478d06428a41d88 ]---

[Xin Long]

On Tue, Jan 16, 2018 at 4:22 AM, syzbot
<syzbot+ed0838d0fa4c4f2b...@syzkaller.appspotmail.com>
wrote:

ipv4 tunnels don't really set dev->hard_header_len properly,
we may should fix it in pppoe by using needed_headroom,
as what it doesn't in arp_create.

@@ -860,16 +861,16 @@ static int pppoe_sendmsg(struct socket *sock,
struct msghdr *m,
if (total_len > (dev->mtu + dev->hard_header_len))
goto end;

+ rlen = LL_RESERVED_SPACE(dev) + dev->needed_tailroom;

- skb = sock_wmalloc(sk, total_len + dev->hard_header_len + 32,
- 0, GFP_KERNEL);
+ skb = sock_wmalloc(sk, total_len + rlen + 32, 0, GFP_KERNEL);
if (!skb) {
error = -ENOMEM;
goto end;
}

/* Reserve space for headers. */
- skb_reserve(skb, dev->hard_header_len);
+ skb_reserve(skb, rlen);

[Guillaume Nault]

On Tue, Jan 16, 2018 at 04:21:40PM +0800, Xin Long wrote:
> ipv4 tunnels don't really set dev->hard_header_len properly,
> we may should fix it in pppoe by using needed_headroom,
> as what it doesn't in arp_create.
>

I'm a bit in doubt about which device needs to be fixed. Should ip_gre
set ->hard_header_len? Or should pppoe take ->needed_headroom into
account in skb_reserve()? I'd favor the later option too, but I haven't
figured out the semantic of these fields precisely enough to justify
this choice.

> @@ -860,16 +861,16 @@ static int pppoe_sendmsg(struct socket *sock,
> struct msghdr *m,
> if (total_len > (dev->mtu + dev->hard_header_len))
> goto end;
>
> + rlen = LL_RESERVED_SPACE(dev) + dev->needed_tailroom;
>
> - skb = sock_wmalloc(sk, total_len + dev->hard_header_len + 32,
> - 0, GFP_KERNEL);
> + skb = sock_wmalloc(sk, total_len + rlen + 32, 0, GFP_KERNEL);
> if (!skb) {
> error = -ENOMEM;
> goto end;
> }
>
> /* Reserve space for headers. */
> - skb_reserve(skb, dev->hard_header_len);
> + skb_reserve(skb, rlen);

Any reason why you include dev->needed_tailroom in skb_reserve()?
BTW, we also have to fix __pppoe_xmit.

What about this patch?


---- >8 ----
diff --git a/drivers/net/ppp/pppoe.c b/drivers/net/ppp/pppoe.c
index 4e1da1645b15..42518af53332 100644
--- a/drivers/net/ppp/pppoe.c
+++ b/drivers/net/ppp/pppoe.c
@@ -842,6 +842,7 @@ static int pppoe_sendmsg(struct socket *sock, struct msghdr *m,
struct pppoe_hdr *ph;
struct net_device *dev;
char *start;
+ int hlen;

lock_sock(sk);
if (sock_flag(sk, SOCK_DEAD) || !(sk->sk_state & PPPOX_CONNECTED)) {

@@ -860,16 +861,16 @@ static int pppoe_sendmsg(struct socket *sock, struct msghdr *m,
if (total_len > (dev->mtu + dev->hard_header_len))
goto end;

-

- skb = sock_wmalloc(sk, total_len + dev->hard_header_len + 32,
- 0, GFP_KERNEL);

+ hlen = LL_RESERVED_SPACE(dev);
+ skb = sock_wmalloc(sk, hlen + sizeof(struct pppoe_hdr) + total_len +
+ dev->needed_tailroom, 0, GFP_KERNEL);

if (!skb) {
error = -ENOMEM;
goto end;
}

/* Reserve space for headers. */
- skb_reserve(skb, dev->hard_header_len);

+ skb_reserve(skb, hlen);
skb_reset_network_header(skb);

skb->dev = dev;
@@ -930,7 +931,7 @@ static int __pppoe_xmit(struct sock *sk, struct sk_buff *skb)
/* Copy the data if there is no space for the header or if it's
* read-only.
*/
- if (skb_cow_head(skb, sizeof(*ph) + dev->hard_header_len))
+ if (skb_cow_head(skb, LL_RESERVED_SPACE(dev) + sizeof(*ph)))
goto abort;

__skb_push(skb, sizeof(*ph));

[Xin Long]

On Sat, Jan 20, 2018 at 1:19 AM, Guillaume Nault <g.n...@alphalink.fr> wrote:
> On Tue, Jan 16, 2018 at 04:21:40PM +0800, Xin Long wrote:
>> ipv4 tunnels don't really set dev->hard_header_len properly,
>> we may should fix it in pppoe by using needed_headroom,
>> as what it doesn't in arp_create.
>>
> I'm a bit in doubt about which device needs to be fixed. Should ip_gre
> set ->hard_header_len? Or should pppoe take ->needed_headroom into
> account in skb_reserve()? I'd favor the later option too, but I haven't
> figured out the semantic of these fields precisely enough to justify
> this choice.

That's also why I haven't posted the patch yet.
(Sorry, I almost forgot this mail.)

>
>> @@ -860,16 +861,16 @@ static int pppoe_sendmsg(struct socket *sock,
>> struct msghdr *m,
>> if (total_len > (dev->mtu + dev->hard_header_len))
>> goto end;
>>
>> + rlen = LL_RESERVED_SPACE(dev) + dev->needed_tailroom;
>>
>> - skb = sock_wmalloc(sk, total_len + dev->hard_header_len + 32,
>> - 0, GFP_KERNEL);
>> + skb = sock_wmalloc(sk, total_len + rlen + 32, 0, GFP_KERNEL);
>> if (!skb) {
>> error = -ENOMEM;
>> goto end;
>> }
>>
>> /* Reserve space for headers. */
>> - skb_reserve(skb, dev->hard_header_len);
>> + skb_reserve(skb, rlen);
> Any reason why you include dev->needed_tailroom in skb_reserve()?
> BTW, we also have to fix __pppoe_xmit.

I noticed them right after I replied, and was about to correct when submitting
and after figuring out the difference between hard_header_len and
needed_headroom.

it's good if you wish to do this with the following patch :-)