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)
