Date: Sat, 29 Aug 2020 12:38:24 +0200 From: Michal Meloun <meloun.michal@gmail.com> To: Warner Losh <imp@bsdimp.com>, Mateusz Guzik <mjguzik@gmail.com> Cc: Warner Losh <imp@freebsd.org>, src-committers <src-committers@freebsd.org>, svn-src-all <svn-src-all@freebsd.org>, svn-src-head <svn-src-head@freebsd.org> Subject: Re: svn commit: r364946 - head/sys/kern Message-ID: <f1a67850-e9e5-d785-6562-972aeb9f1206@gmail.com> In-Reply-To: <CANCZdfqXtKhKhh33ovFQ4_a3tiesRi8-6ZuMTp0yW%2BMzkxWLzA@mail.gmail.com> References: <202008290430.07T4UCM4007928@repo.freebsd.org> <CAGudoHFAkrAykin6ngH=04254J4AmhHk2NmDyGfrUE=wJcxH2A@mail.gmail.com> <CANCZdfqXtKhKhh33ovFQ4_a3tiesRi8-6ZuMTp0yW%2BMzkxWLzA@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On 29.08.2020 12:04, Warner Losh wrote:
> On Sat, Aug 29, 2020 at 1:09 AM Mateusz Guzik <mjguzik@gmail.com> wrote:
>
>> This crashes on boot for me:
>>
>
> I wasn't able to get it to crash on boot for me, but I was able to recreate
> it.
It crashed on ofw based systems where some enumerated devices have not a
suitable driver, see:
---------------------------------------
sysctl_devices: nameunit: root0, descs: System root bus, driver: root
sysctl_devices: nameunit: nexus0, descs: (null), driver: nexus
sysctl_devices: nameunit: ofwbus0, descs: Open Firmware Device Tree,
driver: ofwbus
sysctl_devices: nameunit: pcib0, descs: Nvidia Integrated PCI/PCI-E
Controller, driver: pcib
sysctl_devices: nameunit: simplebus0, descs: Flattened device tree
simple bus, driver: simplebus
sysctl_devices: nameunit: gic0, descs: ARM Generic Interrupt Controller,
driver: gic
sysctl_devices: nameunit: (null), descs: (null), driver:
sysctl_devices: nameunit: lic0, descs: (null), driver: lic
sysctl_devices: nameunit: (null), descs: (null), driver:
sysctl_devices: nameunit: car0, descs: Tegra Clock Driver, driver: car
....
----------------------------------------------------------------------
> Fixed in r364949.Confirmed.
I think it didn't crash on boot for me because
> kldxref failed due to the segment thing so devmatch didn't run which would
> have triggered this bug. devinfo did trigger a very similar crash, and
> r364949 fixes that crash. Even a new kldxref failed due to the too many
> segments thing, so I can't confirm that's what you hit, but I'm pretty sure
> it is...
>
But there is another issue in device_sysctl_handler() (not analyzed yet):
root@tegra210:~ # sysctl dev.cpu.
dev.cpu.3.temperature: 50.5C
dev.cpu.3panic: sbuf_clear makes no sense on sbuf 0xffff00006f21a528
with drain
cpuid = 2
time = 1598696937
KDB: stack backtrace:
db_trace_self() at db_fetch_ksymtab+0x164
pc = 0xffff0000006787f4 lr = 0xffff000000153400
sp = 0xffff00006f21a1b0 fp = 0xffff00006f21a3b0
db_fetch_ksymtab() at vpanic+0x198
pc = 0xffff000000153400 lr = 0xffff00000036b274
sp = 0xffff00006f21a3c0 fp = 0xffff00006f21a420
vpanic() at panic+0x44
pc = 0xffff00000036b274 lr = 0xffff00000036b018
sp = 0xffff00006f21a430 fp = 0xffff00006f21a4e0
panic() at sbuf_clear+0xa0
pc = 0xffff00000036b018 lr = 0xffff0000003c17c8
sp = 0xffff00006f21a4f0 fp = 0xffff00006f21a4f0
sbuf_clear() at sbuf_cpy+0x58
pc = 0xffff0000003c17c8 lr = 0xffff0000003c1ff0
sp = 0xffff00006f21a500 fp = 0xffff00006f21a500
sbuf_cpy() at _gone_in_dev+0x560
pc = 0xffff0000003c1ff0 lr = 0xffff0000003a9078
sp = 0xffff00006f21a510 fp = 0xffff00006f21a570
_gone_in_dev() at sbuf_new_for_sysctl+0x170
pc = 0xffff0000003a9078 lr = 0xffff00000037c1a8
sp = 0xffff00006f21a580 fp = 0xffff00006f21a5a0
sbuf_new_for_sysctl() at kernel_sysctl+0x36c
pc = 0xffff00000037c1a8 lr = 0xffff00000037b63c
sp = 0xffff00006f21a5b0 fp = 0xffff00006f21a630
kernel_sysctl() at userland_sysctl+0xf4
pc = 0xffff00000037b63c lr = 0xffff00000037bc5c
sp = 0xffff00006f21a640 fp = 0xffff00006f21a6d0
userland_sysctl() at sys___sysctl+0x68
pc = 0xffff00000037bc5c lr = 0xffff00000037bb28
sp = 0xffff00006f21a6e0 fp = 0xffff00006f21a790
sys___sysctl() at do_el0_sync+0x4e0
pc = 0xffff00000037bb28 lr = 0xffff000000697918
sp = 0xffff00006f21a7a0 fp = 0xffff00006f21a830
do_el0_sync() at handle_el0_sync+0x90
pc = 0xffff000000697918 lr = 0xffff00000067aa24
sp = 0xffff00006f21a840 fp = 0xffff00006f21a980
handle_el0_sync() at 0x4047764c
pc = 0xffff00000067aa24 lr = 0x000000004047764c
sp = 0xffff00006f21a990 fp = 0x0000ffffffffc250
KDB: enter: panic
[ thread pid 1263 tid 100092 ]
Stopped at 0x40477fb4: undefined 54000042
> Warner
>
>
>> atal trap 12: page fault while in kernel mode
>> cpuid = 0; apic id = 00
>> fault virtual address = 0x0
>> fault code = supervisor read data, page not present
>> instruction pointer = 0x20:0xffffffff805b0a7f
>> stack pointer = 0x28:0xfffffe002366a7f0
>> frame pointer = 0x28:0xfffffe002366a7f0
>> code segment = base 0x0, limit 0xfffff, type 0x1b
>> = DPL 0, pres 1, long 1, def32 0, gran 1
>> processor eflags = interrupt enabled, resume, IOPL = 0
>> current process = 89 (devmatch)
>> trap number = 12
>> panic: page fault
>> cpuid = 0
>> time = 1598692135
>> KDB: stack backtrace:
>> db_trace_self_wrapper() at db_trace_self_wrapper+0x2b/frame
>> 0xfffffe002366a4a0
>> vpanic() at vpanic+0x182/frame 0xfffffe002366a4f0
>> panic() at panic+0x43/frame 0xfffffe002366a550
>> trap_fatal() at trap_fatal+0x387/frame 0xfffffe002366a5b0
>> trap_pfault() at trap_pfault+0x4f/frame 0xfffffe002366a610
>> trap() at trap+0x27d/frame 0xfffffe002366a720
>> calltrap() at calltrap+0x8/frame 0xfffffe002366a720
>> --- trap 0xc, rip = 0xffffffff805b0a7f, rsp = 0xfffffe002366a7f0, rbp
>> = 0xfffffe002366a7f0 ---
>> strlen() at strlen+0x1f/frame 0xfffffe002366a7f0
>> sbuf_cat() at sbuf_cat+0x15/frame 0xfffffe002366a810
>> sysctl_devices() at sysctl_devices+0x104/frame 0xfffffe002366a8a0
>> sysctl_root_handler_locked() at sysctl_root_handler_locked+0x91/frame
>> 0xfffffe002366a8f0
>> sysctl_root() at sysctl_root+0x249/frame 0xfffffe002366a970
>> userland_sysctl() at userland_sysctl+0x170/frame 0xfffffe002366aa20
>> sys___sysctl() at sys___sysctl+0x5f/frame 0xfffffe002366aad0
>> amd64_syscall() at amd64_syscall+0x10c/frame 0xfffffe002366abf0
>> fast_syscall_common() at fast_syscall_common+0xf8/frame 0xfffffe002366abf0
>> --- syscall (202, FreeBSD ELF64, sys___sysctl), rip = 0x80041c0ea, rsp
>> = 0x7fffffffda78, rbp = 0x7fffffffdab0 ---
>> KDB: enter: panic
>> [ thread pid 89 tid 100067 ]
>> Stopped at kdb_enter+0x37: movq $0,0x7e2616(%rip)
>>
>>
>> On 8/29/20, Warner Losh <imp@freebsd.org> wrote:
>>> Author: imp
>>> Date: Sat Aug 29 04:30:12 2020
>>> New Revision: 364946
>>> URL: https://svnweb.freebsd.org/changeset/base/364946
>>>
>>> Log:
>>> Move to using sbuf for some sysctl in newbus
>>>
>>> Convert two different sysctl to using sbuf. First, for all the default
>>> sysctls we implement for each device driver that's attached. This is a
>>> pure sbuf conversion.
>>>
>>> Second, convert sysctl_devices to fill its buffer with sbuf rather
>>> than a hand-rolled crappy thing I wrote years ago.
>>>
>>> Reviewed by: cem, markj
>>> Differential Revision: https://reviews.freebsd.org/D26206
>>>
>>> Modified:
>>> head/sys/kern/subr_bus.c
>>>
>>> Modified: head/sys/kern/subr_bus.c
>>>
>> ==============================================================================
>>> --- head/sys/kern/subr_bus.c Sat Aug 29 04:30:06 2020 (r364945)
>>> +++ head/sys/kern/subr_bus.c Sat Aug 29 04:30:12 2020 (r364946)
>>> @@ -260,36 +260,33 @@ enum {
>>> static int
>>> device_sysctl_handler(SYSCTL_HANDLER_ARGS)
>>> {
>>> + struct sbuf sb;
>>> device_t dev = (device_t)arg1;
>>> - const char *value;
>>> - char *buf;
>>> int error;
>>>
>>> - buf = NULL;
>>> + sbuf_new_for_sysctl(&sb, NULL, 1024, req);
>>> switch (arg2) {
>>> case DEVICE_SYSCTL_DESC:
>>> - value = dev->desc ? dev->desc : "";
>>> + sbuf_cpy(&sb, dev->desc ? dev->desc : "");
>>> break;
>>> case DEVICE_SYSCTL_DRIVER:
>>> - value = dev->driver ? dev->driver->name : "";
>>> + sbuf_cpy(&sb, dev->driver ? dev->driver->name : "");
>>> break;
>>> case DEVICE_SYSCTL_LOCATION:
>>> - value = buf = malloc(1024, M_BUS, M_WAITOK | M_ZERO);
>>> - bus_child_location_str(dev, buf, 1024);
>>> + bus_child_location_sb(dev, &sb);
>>> break;
>>> case DEVICE_SYSCTL_PNPINFO:
>>> - value = buf = malloc(1024, M_BUS, M_WAITOK | M_ZERO);
>>> - bus_child_pnpinfo_str(dev, buf, 1024);
>>> + bus_child_pnpinfo_sb(dev, &sb);
>>> break;
>>> case DEVICE_SYSCTL_PARENT:
>>> - value = dev->parent ? dev->parent->nameunit : "";
>>> + sbuf_cpy(&sb, dev->parent ? dev->parent->nameunit : "");
>>> break;
>>> default:
>>> + sbuf_delete(&sb);
>>> return (EINVAL);
>>> }
>>> - error = SYSCTL_OUT_STR(req, value);
>>> - if (buf != NULL)
>>> - free(buf, M_BUS);
>>> + error = sbuf_finish(&sb);
>>> + sbuf_delete(&sb);
>>> return (error);
>>> }
>>>
>>> @@ -5464,13 +5461,13 @@ SYSCTL_PROC(_hw_bus, OID_AUTO, info,
>> CTLTYPE_STRUCT
>>> |
>>> static int
>>> sysctl_devices(SYSCTL_HANDLER_ARGS)
>>> {
>>> + struct sbuf sb;
>>> int *name = (int *)arg1;
>>> u_int namelen = arg2;
>>> int index;
>>> device_t dev;
>>> struct u_device *udev;
>>> int error;
>>> - char *walker, *ep;
>>>
>>> if (namelen != 2)
>>> return (EINVAL);
>>> @@ -5501,34 +5498,21 @@ sysctl_devices(SYSCTL_HANDLER_ARGS)
>>> udev->dv_devflags = dev->devflags;
>>> udev->dv_flags = dev->flags;
>>> udev->dv_state = dev->state;
>>> - walker = udev->dv_fields;
>>> - ep = walker + sizeof(udev->dv_fields);
>>> -#define CP(src) \
>>> - if ((src) == NULL) \
>>> - *walker++ = '\0'; \
>>> - else { \
>>> - strlcpy(walker, (src), ep - walker); \
>>> - walker += strlen(walker) + 1; \
>>> - } \
>>> - if (walker >= ep) \
>>> - break;
>>> -
>>> - do {
>>> - CP(dev->nameunit);
>>> - CP(dev->desc);
>>> - CP(dev->driver != NULL ? dev->driver->name : NULL);
>>> - bus_child_pnpinfo_str(dev, walker, ep - walker);
>>> - walker += strlen(walker) + 1;
>>> - if (walker >= ep)
>>> - break;
>>> - bus_child_location_str(dev, walker, ep - walker);
>>> - walker += strlen(walker) + 1;
>>> - if (walker >= ep)
>>> - break;
>>> - *walker++ = '\0';
>>> - } while (0);
>>> -#undef CP
>>> - error = SYSCTL_OUT(req, udev, sizeof(*udev));
>>> + sbuf_new(&sb, udev->dv_fields, sizeof(udev->dv_fields),
>> SBUF_FIXEDLEN);
>>> + sbuf_cat(&sb, dev->nameunit);
>>> + sbuf_putc(&sb, '\0');
>>> + sbuf_cat(&sb, dev->desc);
>>> + sbuf_putc(&sb, '\0');
>>> + sbuf_cat(&sb, dev->driver != NULL ? dev->driver->name : '\0');
>>> + sbuf_putc(&sb, '\0');
>>> + bus_child_pnpinfo_sb(dev, &sb);
>>> + sbuf_putc(&sb, '\0');
>>> + bus_child_location_sb(dev, &sb);
>>> + sbuf_putc(&sb, '\0');
>>> + error = sbuf_finish(&sb);
>>> + if (error == 0)
>>> + error = SYSCTL_OUT(req, udev, sizeof(*udev));
>>> + sbuf_delete(&sb);
>>> free(udev, M_BUS);
>>> return (error);
>>> }
>>>
>>
>>
>> --
>> Mateusz Guzik <mjguzik gmail.com>
>>
>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?f1a67850-e9e5-d785-6562-972aeb9f1206>