diff mbox series

[v1] timerqueue: use rb_entry_safe() in timerqueue_getnext()

Message ID 20221027212506.3745164-1-pobrn@protonmail.com
State New
Headers
Series [v1] timerqueue: use rb_entry_safe() in timerqueue_getnext() |

Commit Message

Barnabás Pőcze Oct. 27, 2022, 9:37 p.m. UTC 
  When `timerqueue_getnext()` is called on an empty timerqueue
the returned rb_node pointer will be NULL. Using `rb_entry()`
on a potentially NULL pointer will only - coincidentally - work
if the offset of the rb_node member is 0. This is currently the
case for `timerqueue_node`, but should this ever change,
`timerqueue_getnext()` will no longer work correctly on empty
timerqueues. To avoid this, switch to using `rb_entry_safe()`.

Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
---
 include/linux/timerqueue.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--
2.38.1

Comments

Thomas Gleixner Nov. 14, 2022, 12:17 a.m. UTC | #1 
On Thu, Oct 27 2022 at 21:37, Barnabás Pőcze wrote:

> When `timerqueue_getnext()` is called on an empty timerqueue
> the returned rb_node pointer will be NULL. Using `rb_entry()`
> on a potentially NULL pointer will only - coincidentally - work
> if the offset of the rb_node member is 0. This is currently the
> case for `timerqueue_node`, but should this ever change,
> `timerqueue_getnext()` will no longer work correctly on empty
> timerqueues. To avoid this, switch to using `rb_entry_safe()`.

I agree with the change but not with the argumentation above.

Whatever the current offset of node is does not matter at all,
really. This is a blantant missing NULL pointer check which works by
chance.

So there is no weasel wording justfied about "coincidentally" and "might
not longer work correctly".

Just spell it out that this is a blantant bug and nothing else.

Back then when that code got introduced rb_entry_safe() did not exist at
all so it's even more obvious that this is simply a missing NULL pointer
check, right?

This also requires a Fixes: tag which flags the commit which introduces
this bug.

Thanks,

        tglx
Barnabás Pőcze Nov. 14, 2022, 3:54 p.m. UTC | #2 
Hi


2022. november 14., hétfő 1:17 keltezéssel, Thomas Gleixner írta:

> On Thu, Oct 27 2022 at 21:37, Barnabás Pőcze wrote:
> 
> > When `timerqueue_getnext()` is called on an empty timerqueue
> > the returned rb_node pointer will be NULL. Using `rb_entry()`
> > on a potentially NULL pointer will only - coincidentally - work
> > if the offset of the rb_node member is 0. This is currently the
> > case for `timerqueue_node`, but should this ever change,
> > `timerqueue_getnext()` will no longer work correctly on empty
> > timerqueues. To avoid this, switch to using `rb_entry_safe()`.
> 
> I agree with the change but not with the argumentation above.
> 
> Whatever the current offset of node is does not matter at all,
> really. This is a blantant missing NULL pointer check which works by
> chance.
> 
> So there is no weasel wording justfied about "coincidentally" and "might
> not longer work correctly".
> 
> Just spell it out that this is a blantant bug and nothing else.

I agree, I was just trying to describe why it has not caused any issues yet.

Would this be better?

  When `timerqueue_getnext()` is called on an empty timer queue, it will
  use `rb_entry()` on a NULL pointer, which is invalid. Fix that by using
  `rb_entry_safe()` which handles NULL pointers.
  
  This has not caused any issues so far because the offset of the `rb_node`
  member in `timerqueue_node` is 0, so `rb_entry()` is essentially a no-op.


> 
> Back then when that code got introduced rb_entry_safe() did not exist at
> all so it's even more obvious that this is simply a missing NULL pointer
> check, right?

As far as I can tell it did exist and it was actually used
when the offending change was committed (511885d7061e).


> 
> This also requires a Fixes: tag which flags the commit which introduces
> this bug.
> 
> Thanks,
> 
>         tglx
> 


Regards,
Barnabás Pőcze
Thomas Gleixner Nov. 14, 2022, 7:16 p.m. UTC | #3 
On Mon, Nov 14 2022 at 15:54, Barnabás Pőcze wrote:
> 2022. november 14., hétfő 1:17 keltezéssel, Thomas Gleixner írta:
>> On Thu, Oct 27 2022 at 21:37, Barnabás Pőcze wrote:
>
>   When `timerqueue_getnext()` is called on an empty timer queue, it will
>   use `rb_entry()` on a NULL pointer, which is invalid. Fix that by using
>   `rb_entry_safe()` which handles NULL pointers.
>   
>   This has not caused any issues so far because the offset of the `rb_node`
>   member in `timerqueue_node` is 0, so `rb_entry()` is essentially a
>   no-op.

Yes. Very precise and informative.

>> Back then when that code got introduced rb_entry_safe() did not exist at
>> all so it's even more obvious that this is simply a missing NULL pointer
>> check, right?
>
> As far as I can tell it did exist and it was actually used
> when the offending change was committed (511885d7061e).

Hmm. I must have messed up when searching in the history.

Thanks,

        tglx
diff mbox series

Patch

diff --git a/include/linux/timerqueue.h b/include/linux/timerqueue.h
index 93884086f392..adc80e29168e 100644
--- a/include/linux/timerqueue.h
+++ b/include/linux/timerqueue.h
@@ -35,7 +35,7 @@  struct timerqueue_node *timerqueue_getnext(struct timerqueue_head *head)
 {
 	struct rb_node *leftmost = rb_first_cached(&head->rb_root);

-	return rb_entry(leftmost, struct timerqueue_node, node);
+	return rb_entry_safe(leftmost, struct timerqueue_node, node);
 }

 static inline void timerqueue_init(struct timerqueue_node *node)