Re: Possible ABA in use of llist.h llist_del_first() in tty_buffer and ib_rdma

[Peter Hurley]

On 04/01/2016 02:32 PM, Mathieu Desnoyers wrote:
> ----- On Mar 31, 2016, at 9:58 AM, Peter Hurley peter@xxxxxxxxxxxxxxxxxx wrote:
>> On 03/31/2016 02:40 AM, Mathieu Desnoyers wrote:
>>> CCing LKML.
>>>
>>> ----- On Mar 31, 2016, at 5:39 AM, Mathieu Desnoyers
>>> mathieu.desnoyers@xxxxxxxxxxxx wrote:
>>>
>>>> Hi,
>>>>
>>>> Code review (really: grepping the Linux kernel for
>>>> llist_del_first) leads me to notice two possible ABA issues.
>>>> The first one is in drivers/tty/tty_buffer.c, and is due to
>>>> its use of llist_del_all and llist_del_first without locking
>>>> since commit 809850b7a5 "tty: Use lockless flip buffer free list".
>>>>
>>>> Unfortunately, it appears to do so without respecting the
>>>> locking requirements associated with llist_del_first.
>>>>
>>>> Quoting llist.h:
>>>>
>>>> " * If there are multiple producers and one consumer, llist_add can be
>>>> * used in producers and llist_del_all or llist_del_first can be used
>>>> * in the consumer.
>>
>> The use of llist_del_all in tty_buffer_free_all() is not concurrent with
>> any other use of the free list; the comments for tty_buffer_free_all() even
>> note the special condition.
> 
> This one looks OK indeed.
> 
>>
>> Only the llist_del_first() and llist_add() usage are concurrent, and fwiw,
>> that usage is single-producer/single-consumer.
> 
> I see that tty_buffer_request_room is an exported symbol, and no
> documentation indicate that it should never be called concurrently
> for a struct tty_port. Also, there does not appear to be any locking
> within this function preventing concurrent execution on a struct tty_port.
> Is there some documentation about this interface that I am missing ?

There is little to no documentation on the tty flip buffer interface,
so you're not missing anything there.

The driver-side flip buffer interface is purely single-threaded;
it is exclusively called from interrupt handlers and single-threaded
bottom halves.

None of the functions are atomic, nor is the interface design.

For example, ignoring tty_buffer_request_room() for a moment, consider
how broken concurrent use of tty_insert_flip_string_flags() would be;
input from multiple threads would be overwritten/lost/mixed.

The interface itself is not atomic because tty_flip_buffer_push()
marks the conclusion of input and the hand-off to kworker, which may
already be running at the time.

The tty core doesn't support multi-channel input directly; the driver
is expected to deliver input from each channel to a separate tty,
or mux the inputs before tty_insert_flip_string_fixed_flag().



> If it's possible to call llist_del_first() concurrently, then we can run
> into ABA scenarios, even if llist_add() is protected from concurrent
> llist_add() by a lock.

And way more obvious problems than that, such as I wrote above,
if used concurrently.

Regards,
Peter Hurley