Re: [PATCH -next] serial: core: fix NULL/dangling port_dev on failed re-register

[cuigaosheng <cuigaosheng1@huawei.com>]

Thanks for the review, I have submitted v2 of the patch.

I have tested on x86_64 linux-next (7.2.0-rc7) with KASAN, failslab and
fault injection debugfs enabled;

It takes two unbind rounds to reproduce since
serial8250_unregister_port() first unregisters and then re-registers
the port: the fault injection must hit the re-registration to plant
the stale port_dev, and only the next unbind dereferences it. The
reproducer is a small userspace program that loops over sysfs
bind/unbind of serial8250 and scans /proc/self/fail-nth from 1 to
6000, making the Nth slab allocation on the re-register path fail
with -ENOMEM:

   bind; fail_nth=N; unbind; fail_nth=0; bind; unbind

For fail-nth to reach this path, failslab's ignore-gfp-wait needs
to be cleared first, otherwise GFP_KERNEL allocations are skipped
before the fail-nth check and nothing gets injected.

Without the patch the scan hits the oops quoted in the commit
message. With the patch the full 6000-iteration scan completes
without any crash, the FAULT_INJECTION debug output in dmesg
confirms the injections were actually hitting the path, and the
ports keep working afterwards since a failed re-register is simply
retried on the next bind/unbind round.

Thanks,

On 2026/9/23 20:52, Greg KH wrote:
> On Mon, Sep 14, 2026 at 09:51:54PM +0800, Gaosheng Cui wrote:
>> From: Cui GaoSheng <cuigaosheng1@huawei.com>
>>
>> serial8250_unregister_port() removes a port and then re-registers it
>> against the legacy serial8250 device when that device is still
>> present, ignoring the return value of uart_add_one_port(). When the
>> re-register fails, the error path of serial_core_register_port()
>> removes the port device but leaves port->port_dev dangling. A port
>> that was never successfully registered (e.g. serial_core_port_device_add()
>> failed) also reaches the unregister path with port->port_dev unset.
>> serial8250 matches ports on ->dev, so such half-registered slots
>> still reach serial_core_unregister_port() on the next unbind:
>>
>>    Oops: general protection fault ... SMP KASAN PTI
>>    KASAN: null-ptr-deref in range [0x40-0x47]
>>    RIP: serial_core_unregister_port+0xe5/0xa00
>>      serial8250_unregister_port+0x103
>>      serial8250_remove+0x61
>>      device_release_driver_internal+0x38c
>>      unbind_store+0xd1
>>      kernfs_fop_write_iter+0x329
>>      vfs_write+0x5d0
>>      ksys_write+0xf7
>>      do_syscall_64+0xdd
>>      entry_SYSCALL_64_after_hwframe+0x77
>>
>> Fix the error path to clear port->port_dev and restore the state slot
>> (guarded by line range) to the "not registered" state, and bail out of
>> serial_core_unregister_port() when port_dev is NULL (either dangling
>> from a failed re-register or never set).
>>
>> Fixes: 84a9582fd203 ("serial: core: Start managing serial controllers to enable runtime PM")
>> Signed-off-by: Cui GaoSheng <cuigaosheng1@huawei.com>
>> ---
> Did you forget the Assited-by: tag?
>
>
>>   drivers/tty/serial/serial_core.c | 25 ++++++++++++++++++++++++-
>>   1 file changed, 24 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
>> index 95774b0f1484..62d93c5964e2 100644
>> --- a/drivers/tty/serial/serial_core.c
>> +++ b/drivers/tty/serial/serial_core.c
>> @@ -3358,6 +3358,17 @@ int serial_core_register_port(struct uart_driver *drv, struct uart_port *port)
>>   
>>   err_unregister_port_dev:
>>   	serial_base_port_device_remove(port->port_dev);
>> +	/*
>> +	 * serial_base_port_device_remove() dropped the last reference to the
>> +	 * port device, so leave no dangling ->port_dev behind. Restore the
>> +	 * state slot to the same "not registered" state that
>> +	 * serial_core_remove_one_port() produces on the normal path; the
>> +	 * line range guard also covers the -EINVAL exit of
>> +	 * serial_core_add_one_port() where line >= drv->nr.
>> +	 */
>> +	port->port_dev = NULL;
>> +	if (port->line < drv->nr && drv->state[port->line].uart_port == port)
>> +		drv->state[port->line].uart_port = NULL;
>
> How was this tested?
>
>
>>   
>>   err_unregister_ctrl_dev:
>>   	serial_base_ctrl_device_remove(new_ctrl_dev);
>> @@ -3373,11 +3384,23 @@ void serial_core_unregister_port(struct uart_driver *drv, struct uart_port *port
>>   {
>>   	struct device *phys_dev = port->dev;
>>   	struct serial_port_device *port_dev = port->port_dev;
>> -	struct serial_ctrl_device *ctrl_dev = serial_core_get_ctrl_dev(port_dev);
>> +	struct serial_ctrl_device *ctrl_dev;
>>   	int ctrl_id = port->ctrl_id;
>>   
>> +	/*
>> +	 * A port may be unregistered even though it was never successfully
>> +	 * registered (e.g. an allocation failure aborted serial_core_register_port()
>> +	 * part-way). Drivers like serial8250 remove ports matching on ->dev, so
>> +	 * those half-registered ports still reach us with port_dev == NULL. Do not
>> +	 * dereference it.
>> +	 */
>> +	if (!port_dev)
>> +		return;
> What keeps port_dev from not changing right after you read it?
>
> thanks,
>
> greg k-h
>
> .