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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.02.2025 12:58, Jürgen Groß wrote:
> On 17.02.25 12:39, Jan Beulich wrote:
>> 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.
> 
> The rcu_dereference() is basically just for documentation purposes. If needed
> by an arch, it can add barriers, too, but according to the comments those 
> would
> be needed on alpha only. The returned value is always the original pointer
> value. See the comment block in xen/include/xen/rcupdate.h

Note the "This pointer may later be safely dereferenced" in there. As said,
we'd be fine if we used the return value instead of the original "pos". Yet
we don't. We effectively assume that the macro expands to just the passed
in pointer, with no barriers or anything else.

Anyway, since - as you validly say - this is a pre-existing issue, let's put
it on the side right here.

Jan