[PATCH v2] posix-clock: posix-clock: Fix unbalanced locking in pc_clock_settime()

[PATCH v2] posix-clock: posix-clock: Fix unbalanced locking in pc_clock_settime()

[Jinjie Ruan]

If get_clock_desc() succeeds, it calls fget() for the clockid's fd,
and get the clk->rwsem read lock, so the error path should release
the lock to make the lock balance and fput the clockid's fd to make
the refcount balance and release the fd related reosurce.

However the below commit left the error path locked behind resulting in
unbalanced locking. Check timespec64_valid_strict() before
get_clock_desc() to fix it, because the "ts" is not changed
after that.

Fixes: d8794ac20a29 ("posix-clock: Fix missing timespec64 check in pc_clock_settime()")
Acked-by: Richard Cochran <richardcochran@gmail.com>
Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
---
v2:
- No longer against -next.
- Update the commit message.
- Add Acked-by.
---
 kernel/time/posix-clock.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/kernel/time/posix-clock.c b/kernel/time/posix-clock.c
index 316a4e8c97d3..1af0bb2cc45c 100644
--- a/kernel/time/posix-clock.c
+++ b/kernel/time/posix-clock.c
@@ -309,6 +309,9 @@ static int pc_clock_settime(clockid_t id, const struct timespec64 *ts)
 	struct posix_clock_desc cd;
 	int err;
 
+	if (!timespec64_valid_strict(ts))
+		return -EINVAL;
+
 	err = get_clock_desc(id, &cd);
 	if (err)
 		return err;
@@ -318,9 +321,6 @@ static int pc_clock_settime(clockid_t id, const struct timespec64 *ts)
 		goto out;
 	}
 
-	if (!timespec64_valid_strict(ts))
-		return -EINVAL;
-
 	if (cd.clk->ops.clock_settime)
 		err = cd.clk->ops.clock_settime(cd.clk, ts);
 	else
-- 
2.34.1

Re: [PATCH v2] posix-clock: posix-clock: Fix unbalanced locking in pc_clock_settime()

[Thomas Gleixner]

On Fri, Oct 18 2024 at 18:07, Jinjie Ruan wrote:
 If get_clock_desc() succeeds, it calls fget() for the clockid's fd,
> and get the clk->rwsem read lock, so the error path should release
> the lock to make the lock balance and fput the clockid's fd to make
> the refcount balance and release the fd related reosurce.
>
> However the below commit left the error path locked behind resulting in
> unbalanced locking. Check timespec64_valid_strict() before
> get_clock_desc() to fix it, because the "ts" is not changed
> after that.
>
> Fixes: d8794ac20a29 ("posix-clock: Fix missing timespec64 check in
> pc_clock_settime()")

Jakub, I expect _you_ are going to pick this up and explain to Linus and
the stable people why we need a fix for the rushed in "fix".

Thanks,

        tglx

Re: [PATCH v2] posix-clock: posix-clock: Fix unbalanced locking in pc_clock_settime()

[Anna-Maria Behnsen]

Jinjie Ruan <ruanjinjie@huawei.com> writes:

> If get_clock_desc() succeeds, it calls fget() for the clockid's fd,
> and get the clk->rwsem read lock, so the error path should release
> the lock to make the lock balance and fput the clockid's fd to make
> the refcount balance and release the fd related reosurce.

There is a typo: reosurce

>
> However the below commit left the error path locked behind resulting in
> unbalanced locking. Check timespec64_valid_strict() before
> get_clock_desc() to fix it, because the "ts" is not changed
> after that.
>
> Fixes: d8794ac20a29 ("posix-clock: Fix missing timespec64 check in pc_clock_settime()")
> Acked-by: Richard Cochran <richardcochran@gmail.com>
> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>

Acked-by: Anna-Maria Behnsen <anna-maria@linutronix.de>

Re: [PATCH v2] posix-clock: posix-clock: Fix unbalanced locking in pc_clock_settime()

[Paolo Abeni]

On 10/19/24 01:26, Thomas Gleixner wrote:
> On Fri, Oct 18 2024 at 18:07, Jinjie Ruan wrote:
>  If get_clock_desc() succeeds, it calls fget() for the clockid's fd,
>> and get the clk->rwsem read lock, so the error path should release
>> the lock to make the lock balance and fput the clockid's fd to make
>> the refcount balance and release the fd related reosurce.
>>
>> However the below commit left the error path locked behind resulting in
>> unbalanced locking. Check timespec64_valid_strict() before
>> get_clock_desc() to fix it, because the "ts" is not changed
>> after that.
>>
>> Fixes: d8794ac20a29 ("posix-clock: Fix missing timespec64 check in
>> pc_clock_settime()")
> 
> Jakub, I expect _you_ are going to pick this up and explain to Linus and
> the stable people why we need a fix for the rushed in "fix".

I'm sorry, I noticed this patch right now thanks to Anna-Maria head-up
on netdev.
I'll merge it into net before this week PR.

Again, I'm sorry for this mess.

Cheers,

Paolo

Re: [PATCH v2] posix-clock: posix-clock: Fix unbalanced locking in pc_clock_settime()

[Paolo Abeni]

On 10/22/24 17:42, Paolo Abeni wrote:
> On 10/19/24 01:26, Thomas Gleixner wrote:
>> On Fri, Oct 18 2024 at 18:07, Jinjie Ruan wrote:
>>  If get_clock_desc() succeeds, it calls fget() for the clockid's fd,
>>> and get the clk->rwsem read lock, so the error path should release
>>> the lock to make the lock balance and fput the clockid's fd to make
>>> the refcount balance and release the fd related reosurce.
>>>
>>> However the below commit left the error path locked behind resulting in
>>> unbalanced locking. Check timespec64_valid_strict() before
>>> get_clock_desc() to fix it, because the "ts" is not changed
>>> after that.
>>>
>>> Fixes: d8794ac20a29 ("posix-clock: Fix missing timespec64 check in
>>> pc_clock_settime()")
>>
>> Jakub, I expect _you_ are going to pick this up and explain to Linus and
>> the stable people why we need a fix for the rushed in "fix".
> 
> I'm sorry, I noticed this patch right now thanks to Anna-Maria head-up
> on netdev.
> I'll merge it into net before this week PR.
> 
> Again, I'm sorry for this mess.

The patch is now merged into the 'net' tree:

https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=6e62807c7fbb3c758d233018caf94dfea9c65dbd

and I'll send it to Linus with tomorrow PR.

I forgot to mention Jakub could not reply as he is off-the-grid for a
few more days.

Cheers,

Paolo