Re: [PATCH 1/2] xen/list: avoid UB in list iterators

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.02.2025 12:16, Juergen Gross wrote:
> On 17.02.25 10:47, Jan Beulich wrote:
>> On 16.02.2025 11:23, Juergen Gross wrote:
>>> @@ -556,11 +590,11 @@ static inline void list_splice_init(struct list_head 
>>> *list,
>>>    * @head:   the head for your list.
>>>    * @member: the name of the list_struct within the struct.
>>>    */
>>> -#define list_for_each_entry_safe(pos, n, head, member)                  \
>>> -    for ((pos) = list_entry((head)->next, typeof(*(pos)), member),      \
>>> -         (n) = list_entry((pos)->member.next, typeof(*(pos)), member);  \
>>> -         &(pos)->member != (head);                                      \
>>> -         (pos) = (n), (n) = list_entry((n)->member.next, typeof(*(n)), 
>>> member))
>>> +#define list_for_each_entry_safe(pos, n, head, member)                     
>>> \
>>> +    for ( (pos) = list_first_entry_or_null(head, typeof(*(pos)), member),  
>>> \
>>> +          (n) = (pos) ? list_next_entry_or_null(head, pos, member) : NULL; 
>>> \
>>
>> n can end up being NULL here even if pos isn't. Then ...
>>
>>> +          pos;                                                             
>>> \
>>> +          (pos) = (n), (n) = list_next_entry_or_null(head, n, member) )
>>
>> ... you can't use list_next_entry_or_null() on it.
> 
> Ah, indeed.
> 
> What would you prefer? Handling that in the *_safe() iterator macros, or
> allowing the *_entry_or_null() macros to be called with a NULL parameter?

I'd prefer the former, but I probably wouldn't mind the latter.

>>> @@ -655,10 +689,10 @@ static inline void list_splice_init(struct list_head 
>>> *list,
>>>    * the _rcu list-mutation primitives such as list_add_rcu()
>>>    * as long as the traversal is guarded by rcu_read_lock().
>>>    */
>>> -#define list_for_each_entry_rcu(pos, head, member)                      \
>>> -    for ((pos) = list_entry((head)->next, typeof(*(pos)), member);      \
>>> -         &rcu_dereference(pos)->member != (head);                       \
>>> -         (pos) = list_entry((pos)->member.next, typeof(*(pos)), member))
>>> +#define list_for_each_entry_rcu(pos, head, member)                        \
>>> +    for ( (pos) = list_first_entry_or_null(head, typeof(*(pos)), member); \
>>> +          rcu_dereference(pos);                                           \
>>> +          (pos) = list_next_entry_or_null(head, pos, member) )
>>
>> Don't you need a list_next_entry_or_null_rcu() flavor here, using
>> rcu_dereference() on the passed in pos for the (pos)->member.next deref?
> 
> Isn't the "rcu_dereference(pos);" all what is needed for the current 
> iteration?

Reading Linux'es rcu_dereference.rst, my understanding is that one of them
would suffice if then we used its result rather than the original pointer.
Then again RCU has been somewhat opaque to me for all the years ...

> Otherwise today's implementation would suffer from the same problem IMHO.

That's the impression I'm (now) having.

Jan