答复: [PATCH,v3] arm64: fix the illegal address access in some cases

[Guodeqing (A)]

> -----邮件原件-----
> 发件人: Robin Murphy [mailto:robin.murphy at arm.com]
> 发送时间: Thursday, July 30, 2020 20:02
> 收件人: Guodeqing (A) <geffrey.guo at huawei.com>;
> catalin.marinas at arm.com
> 抄送: will at kernel.org; luke.starrett at broadcom.com;
> linux-arm-kernel at lists.infradead.org
> 主题: Re: [PATCH,v3] arm64: fix the illegal address access in some cases
> 
> On 2020-07-30 12:24, guodeqing wrote:
> > If do the following test in the arm64 VM, a panic will be produced.
> > $ ifconfig eth0 up
> > $ ip netns add ns1
> > $ ip link add gw link eth0 type ipvlan $ ip addr add 168.16.0.1/24 dev
> > gw $ ip link set dev gw up $ ip link add ip1 link eth0 type ipvlan $
> > ip link set ip1 netns ns1 $ ip netns exec ns1 ip link set ip1 up $ ip
> > netns exec ns1 ip addr add 168.16.0.2/24 dev ip1 $ ip netns exec ns1
> > ip link set lo up $ ip netns exec ns1 ip addr add 127.0.0.1/8 dev lo $
> > ip netns exec ns1 tc qdisc add dev ip1 root netem corrupt 100% $ ip
> > netns exec ns1 ping 168.16.0.1
> >
> >   | Unable to handle kernel paging request at virtual address
> >   | Internal error: Oops: 96000007 [#1] SMP
> >   | CPU: 1 PID: 525 Comm: ping Not tainted 5.8.0-rc6+ #3
> >   | pstate: 20400005 (nzCv daif +PAN -UAO BTYPE=--)
> >   | pc : __ip_local_out+0x84/0x188
> >   | lr : ip_local_out+0x34/0x68 sp : ffff800013263440
> >   | x29: ffff800013263440 x28: 0000000000000001
> >   | x27: ffff8000111d2018 x26: ffff8000114cba80
> >   | x25: ffff0000ec4e7400 x24: 0000000000000000
> >   | x23: 0000000000000062 x22: ffff8000114c9000
> >   | x21: ffff0000d97ac600 x20: ffff0000ec519000
> >   | x19: ffff8000115b5bc0 x18: 0000000000000000
> >   | x17: 0000000000000000 x16: 0000000000000000
> >   | x15: 0000000000000000 x14: 0000000000000000
> >   | x13: 0000000000000000 x12: 0000000000000001
> >   | x11: ffff800010d21838 x10: 0000000000000001
> >   | x9 : 0000000000000001 x8 : 0000000000000000
> >   | x7 : 0000000000000000 x6 : ffff0000ec4e5e00
> >   | x5 : 024079ca54000184 x4 : ffff0000ec4e5e10
> >   | x3 : 0000000000000000 x2 : ffff0004ec4e5e20
> >   | x1 : ffff0000f85f0000 x0 : 031d079626a9c7ae
> >   | Call trace:
> >   |  __ip_local_out+0x84/0x188
> >   |  ip_local_out+0x34/0x68
> >   |  ipvlan_queue_xmit+0x548/0x5c0
> >   |  ipvlan_start_xmit+0x2c/0x90
> >   |  dev_hard_start_xmit+0xb4/0x260
> >   |  sch_direct_xmit+0x1b4/0x550
> >   |  __qdisc_run+0x140/0x648
> >   |  __dev_queue_xmit+0x6a4/0x8b8
> >   |  dev_queue_xmit+0x24/0x30
> >   |  ip_finish_output2+0x324/0x580
> >   |  __ip_finish_output+0x130/0x218
> >   |  ip_finish_output+0x38/0xd0
> >   |  ip_output+0xb4/0x130
> >
> > Here I add the check of the ihl value to fix the problem.
> 
> Can you confirm that the inline patch I sent also fixes it? I'd prefer that as it has
> no impact for correct cases.

I do a test, your patch aslo solve the problem. your patch is better which has no condition jugement.

> 
> It would also have been useful to explain *why* this case causes a crash, since
> it's not all that obvious - having eventually figured it out by inspection, and
> knowing that compilers like to transform index comparisons into pointer
> comparisons, x4 and x2 in the register dump then start to stand out as the
> smoking gun, but it took me a while to get there, since the nature of your fix
> implies that accessing within the assumed bounds of iph itself was a problem,
> when in fact it isn't.
> 
> > Fixes: 0e455d8e80aa (arm64: Implement optimised IP checksum helpers)
> > Signed-off-by: guodeqing <geffrey.guo at huawei.com>
> > ---
> >   arch/arm64/include/asm/checksum.h | 3 +++
> >   1 file changed, 3 insertions(+)
> >
> > diff --git a/arch/arm64/include/asm/checksum.h
> > b/arch/arm64/include/asm/checksum.h
> > index b6f7bc6..702ac89 100644
> > --- a/arch/arm64/include/asm/checksum.h
> > +++ b/arch/arm64/include/asm/checksum.h
> > @@ -25,6 +25,9 @@ static inline __sum16 ip_fast_csum(const void *iph,
> unsigned int ihl)
> >   	__uint128_t tmp;
> >   	u64 sum;
> >
> > +	if (WARN_ON_ONCE(ihl < 5))
> 
> My reading of the netdev discussion is that if we're expected to be robust to
> this due to possible Rx packet corruption, then WARN probably isn't
> appropriate.
> 
> Robin.
> 
> > +		ihl = 5;
> > +
> >   	tmp = *(const __uint128_t *)iph;
> >   	iph += 16;
> >   	ihl -= 4;
> >