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

[Jürgen Groß <jgross@xxxxxxxx>]

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


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature