RE: [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr

[David Laight]

From: Linus Torvalds
> Sent: 01 March 2022 19:07
> On Mon, Feb 28, 2022 at 2:29 PM James Bottomley
> <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx> wrote:
> >
> > However, if the desire is really to poison the loop variable then we
> > can do
> >
> > #define list_for_each_entry(pos, head, member)                          \
> >         for (pos = list_first_entry(head, typeof(*pos), member);        \
> >              !list_entry_is_head(pos, head, member) && ((pos = NULL) == NULL;                   \
> >              pos = list_next_entry(pos, member))
> >
> > Which would at least set pos to NULL when the loop completes.
> 
> That would actually have been excellent if we had done that
> originally. It would not only avoid the stale and incorrectly typed
> head entry left-over turd, it would also have made it very easy to
> test for "did I find an entry in the loop".
> 
> But I don't much like it in the situation we are now.
> 
> Why? Mainly because it basically changes the semantics of the loop
> _without_ any warnings about it.  And we don't actually get the
> advantage of the nicer semantics, because we can't actually make code
> do
> 
>         list_for_each_entry(entry, ....) {
>                 ..
>         }
>         if (!entry)
>                 return -ESRCH;
>         .. use the entry we found ..
> 
> because that would be a disaster for back-porting, plus it would be a
> flag-day issue (ie we'd have to change the semantics of the loop at
> the same time we change every single user).
> 
> So instead of that simple "if (!entry)", we'd effectively have to
> continue to use something that still works with the old world order
> (ie that "if (list_entry_is_head())" model).
> 
> So we couldn't really take _advantage_ of the nicer semantics, and
> we'd not even get a warning if somebody does it wrong - the code would
> just silently do the wrong thing.
> 
> IOW: I don't think you are wrong about that patch: it would solve the
> problem that Jakob wants to solve, and it would have absolutely been
> much better if we had done this from the beginning. But I think that
> in our current situation, it's actually a really fragile solution to
> the "don't do that then" problem we have.

Can it be resolved by making:
#define list_entry_is_head(pos, head, member) ((pos) == NULL)
and double-checking that it isn't used anywhere else (except in
the list macros themselves).

The odd ones I just found are fs/locks.c mm/page_reporting.c
security/apparmor/apparmorfs.c (3 times)

net/xfrm/xfrm_ipcomp.c#L244 is buggy.
(There is a WARN_ON() then it just carries on regardless!)

There are only about 25 uses of list_entry_is_head().

There are a lot more places where these lists seem to be scanned by hand.
I bet a few of those aren't actually right either.

(Oh at 3am this morning I thought it was a different list type
that could have much the same problem!)

Another plausible solution is a variant of list_foreach_entry()
that does set the 'entry' to NULL at the end.
Then code can be moved over in stages.
I'd reorder the arguments as well as changing the name!

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)