Re: [net RFC v1 1/1] page_pool: fix NULL dereference crash

Alexei Starovoitov
2022-01-22T02:48:08Z

On Fri, Jan 21, 2022 at 6:20 PM Colin Foster
<colin.foster@in-advantage.com> wrote:
>
> On Fri, Jan 21, 2022 at 05:13:28PM -0800, Alexei Starovoitov wrote:
> > On Fri, Jan 21, 2022 at 4:57 PM Colin Foster
> > <colin.foster@in-advantage.com> wrote:
> > >
> > > Check for the existence of page pool params before dereferencing. This can
> > > cause crashes in certain conditions.
> >
> > In what conditions?
> > Out of tree driver?
>
> Hi Alexei,
>
> Thanks for the quick response.
>
> I'm actively working on a DSA driver that is currently out-of-tree, but
> trying to get it into mainline. But I'm not convinced that's the
> problem...
>
> I'm using a beagelebone black with the cpsw_new driver. There are two
> tweaks to that driver: the default vlan port is 10 and 11 so there's no
> conflict between cpsw_new and DSA, and the ndev->max_mtu / rx_packet_max
> have been increased to 1600 to allow for DSA frames larger than the
> standard MTU of 1500.
>
> My focus is on the DSA driver, but the crash happens as soon as I run
> "ip link set eth0 up" which is invoking the cpsw_new driver. Therefore I
> suspect that the issue is not directly related to the DSA section
> (ocelot / felix, much of which uses code that is mainline)
>
> As I suggested, I haven't dug into what is going on around the
> page_pool yet. If there is something that is pre-loading memory at 1500
> byte intervals and I broke that, that's entirely on me.
>
> [ removes 1600 byte MTU patch and pool patch ]
>
> I can confirm it still crashes when I don't modify the MTU in the
> cpsw_new driver... so that doesn't seem to be it. That crash log is
> below.
>
>
> # ip link set eth0 up
> [ 18.074704] cpsw-switch 4a100000.switch: starting ndev. mode: dual_mac
> [ 18.174286] SMSC LAN8710/LAN8720 4a101000.mdio:00: attached PHY driver (mii_bus:phy_addr=4a101000.mdio:00, irq=POLL)
> [ 18.185458] 8<--- cut here ---
> [ 18.188554] Unable to handle kernel paging request at virtual address c3104440
> [ 18.195819] [c3104440] *pgd=8300041e(bad)
> [ 18.199885] Internal error: Oops: 8000000d [#1] SMP ARM
> [ 18.205148] Modules linked in:
> [ 18.208233] CPU: 0 PID: 168 Comm: ip Not tainted 5.16.0-05302-g8bd405e6e8a0-dirty #265
> [ 18.216201] Hardware name: Generic AM33XX (Flattened Device Tree)
> [ 18.222328] PC is at 0xc3104440
> [ 18.225500] LR is at __page_pool_alloc_pages_slow+0xbc/0x2e0
> [ 18.231222] pc : [<c3104440>] lr : [<c0ee06c8>] psr: a00b0013
> [ 18.237523] sp : c3104440 ip : 00000020 fp : c219e580
> [ 18.242778] r10: c1a04d54 r9 : 00000000 r8 : 00000000
> [ 18.248032] r7 : c36b9000 r6 : 00000000 r5 : c36b9084 r4 : 00000000
> [ 18.254595] r3 : c07a399c r2 : 00000000 r1 : c325784c r0 : dfa48bc0
> [ 18.261162] Flags: NzCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none
> [ 18.268343] Control: 10c5387d Table: 836f0019 DAC: 00000051
> [ 18.274119] Register r0 information: non-slab/vmalloc memory
> [ 18.279825] Register r1 information: non-slab/vmalloc memory
> [ 18.285523] Register r2 information: NULL pointer
> [ 18.290260] Register r3 information: non-slab/vmalloc memory
> [ 18.295957] Register r4 information: NULL pointer
> [ 18.300693] Register r5 information: slab kmalloc-1k start c36b9000 pointer offset 132 size 1024
> [ 18.309569] Register r6 information: NULL pointer
> [ 18.314306] Register r7 information: slab kmalloc-1k start c36b9000 pointer offset 0 size 1024
> [ 18.322999] Register r8 information: NULL pointer
> [ 18.327736] Register r9 information: NULL pointer
> [ 18.332473] Register r10 information: non-slab/vmalloc memory
> [ 18.338257] Register r11 information: slab kmalloc-4k start c219e000 pointer offset 1408 size 4096
> [ 18.347301] Register r12 information: non-paged memory
> [ 18.352475] Process ip (pid: 168, stack limit = 0x7eb0d4ab)
> [ 18.358089] Stack: (0xc3104440 to 0xc3258000)
> (too big a stack to show)
>
>
> I can confirm that it crashes on net-next/master as well:
> commit fe8152b38d3a, using the same DTB that defines the cpsw_new port
> as the DSA master. Relevant DTS snippet from my in-development driver:
>
> +&spi0 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + status = "okay";
> +
> + ocelot-chip@0 {
> + compatible = "mscc,vsc7512_mfd_spi";
> + spi-max-frequency = <2500000>;
> + reg = <0>;
> +
> + ethernet-switch@0 {
> + compatible = "mscc,vsc7512-ext-switch";
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + port@0 {
> + reg = <0>;
> + label = "cpu";
> + status = "okay";
> + ethernet = <&mac_sw>;
> + phy-handle = <&sw_phy0>;
> + phy-mode = "internal";
> + };
>
>
> I was hoping for an "oh, if a switch is set up in DSA the page_pool gets
> set up this way" type scenario. I fully understand that might not be the
> case, and the issue could be in something I'm doing incorrectly - it
> certainly wouldn't be the first time.
>
> If this patch doesn't make sense I can look deeper. As mentioned, I'm
> working to get this accepted upstream, so I'll have to figure it out one
> way or another.

With !pool tweak the patch makes sense.

Toke, wdyt?