Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Thomas Gleixner <tglx@linutronix.de>
To:     =?utf-8?Q?Barnab=C3=A1s_P=C5=91cze?= <pobrn@protonmail.com>,
        linux-kernel@vger.kernel.org
Cc:     John Stultz <jstultz@google.com>
Subject: Re: [PATCH v1] timerqueue: use rb_entry_safe() in timerqueue_getnext()
In-Reply-To: <20221027212506.3745164-1-pobrn@protonmail.com>
References: <20221027212506.3745164-1-pobrn@protonmail.com>
Date:   Mon, 14 Nov 2022 01:17:00 +0100
Message-ID: <87v8nis83n.ffs@tglx>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Thu, Oct 27 2022 at 21:37, Barnab=C3=A1s P=C5=91cze 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