linux-kernel - Re: [PATCH v5 3/3] w1: add UART w1 bus driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date: Fri, 2 Feb 2024 20:23:30 +0100
From: Christoph Winklhofer <cj.winklhofer@...il.com>
To: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
Cc: Krzysztof Kozlowski <krzk@...nel.org>, Rob Herring <robh+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>, Rob Herring <robh@...nel.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Jiri Slaby <jirislaby@...nel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
	Jonathan Corbet <corbet@....net>, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-serial@...r.kernel.org,
	linux-doc@...r.kernel.org
Subject: Re: [PATCH v5 3/3] w1: add UART w1 bus driver

On Thu, Feb 01, 2024 at 10:35:32AM +0100, Krzysztof Kozlowski wrote:
> On 01/02/2024 08:29, Christoph Winklhofer wrote:
> >>> +
> >>> +static void w1_uart_remove(struct serdev_device *serdev)
> >>> +{
> >>> +	struct w1_uart_device *w1dev = serdev_device_get_drvdata(serdev);
> >>> +
> >>> +	mutex_lock(&w1dev->mutex);
> >>> +
> >>> +	w1_remove_master_device(&w1dev->bus);
> >>> +
> >>> +	mutex_unlock(&w1dev->mutex);
> >>
> >> This is still suspicious. You do not have serdev_device_close and you
> >> want to protect from concurrent access but it looks insufficient.
> >>
> >> This code assumes that:
> >>
> >> w1_uart_remove()
> >>   <-- here concurrent read/write might start
> >>   mutex_lock()
> >>   w1_remove_master_device()
> >>   mutex_unlock()
> >>   <-- now w1_uart_serdev_tx_rx() or w1_uart_serdev_receive_buf() can be
> >> executed, but device is removed. So what's the point of the mutex here?
> >>
> >> What exactly is protected by the mutex? So far it looks like only some
> >> contents of w1dev, but it does not matter, because it that memory is
> >> still valid at this point.
> >>
> >> After describing what is protected we can think whether it is really
> >> protected...
> >>
> >>
> >>>
> >>
> >> Best regards,
> >> Krzysztof
> >>
> > 
> > Yes, it is still suspicious, sorry..
> > 
> > After w1_uart_remove, serdev is closed and w1dev is released. Therefore
> > the w1-callback (w1_uart_serdev_tx_rx) must be finished before returning
> 
> I did not even go to end of w1_uart_remove(). In my code above, that
> thread is still in w1_uart_remove(), just after unlocking mutex.
> 

Ok, I looked more closely at the underlying w1-code and think it is
sufficient to only call w1_remove_master_device() in w1_uart_remove.
This function waits until all slaves have finished their work, so
w1_uart_serdev_tx_rx finished too. The lock is not required.

Hence, I will use the mutex only to protected concurrent access of
serdev and w1-callbacks (for rx_byte and rx_err).

> > from w1_uart_remove. That was the intention with the lock and trylock.
> 
> Then it does not look really protected. To be honest, w1-gpio and other
> drivers also might have a race here. You can test it by adding long
> sleeps in read/write paths and then trying to unbind device. Maybe this
> should be fixed everywhere, but this mutex here brings more questions.
> 

I added a delay, unbind takes longer but works without problems when its
done during a temperature read.

IMO also the other drivers should be fine. From w1_int.c: The w1_master
is ref-counted and released when it is unused (2). In w1_slave_detach
(1), the slaves decrement this count, perform specific clean up (in
remove_slave) and remove sysfs entries.

w1_int.c:

void __w1_remove_master_device(struct w1_master *dev)
..
	list_for_each_entry_safe(sl, sln, &dev->slist, w1_slave_entry) {
		mutex_unlock(&dev->list_mutex);
		w1_slave_detach(sl); 		(1)
..
	while (atomic_read(&dev->refcnt)) { 	(2)
..	
	}
..
	w1_free_dev(dev);

For example w1_therm waits in remove_slave until its w1-operations are
done. The other slave-drivers (e.g. w1_ds2405.c) use w1-operations in
their sysfs-callback and I suppose that sysfs-removal waits until the
sysfs-read is finished.

Kind regards,
Christoph