There is a double unlock on mutex. This can cause undefined behaviour.
Signed-off-by: Abhinav Singh <[email protected]>
---
net/ipv4/inet_connection_sock.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
index aeebe8816689..f11fe8c727a4 100644
--- a/net/ipv4/inet_connection_sock.c
+++ b/net/ipv4/inet_connection_sock.c
@@ -597,7 +597,6 @@ int inet_csk_get_port(struct sock *sk, unsigned short snum)
}
if (head2_lock_acquired)
spin_unlock(&head2->lock);
- spin_unlock_bh(&head->lock);
return ret;
}
EXPORT_SYMBOL_GPL(inet_csk_get_port);
--
2.39.2
On 10/10/23 15:46, Abhinav Singh wrote:
> There is a double unlock on mutex. This can cause undefined behaviour.
Where is the double unlock of head->lock (which is a spinlock and not a
mutex, btw)?
>
> Signed-off-by: Abhinav Singh <[email protected]>
> ---
> net/ipv4/inet_connection_sock.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
> index aeebe8816689..f11fe8c727a4 100644
> --- a/net/ipv4/inet_connection_sock.c
> +++ b/net/ipv4/inet_connection_sock.c
> @@ -597,7 +597,6 @@ int inet_csk_get_port(struct sock *sk, unsigned short snum)
> }
> if (head2_lock_acquired)
> spin_unlock(&head2->lock);
> - spin_unlock_bh(&head->lock);
> return ret;
> }
> EXPORT_SYMBOL_GPL(inet_csk_get_port);
--
Florian
From: Florian Fainelli <[email protected]>
Date: Tue, 10 Oct 2023 15:51:13 -0700
> On 10/10/23 15:46, Abhinav Singh wrote:
> > There is a double unlock on mutex. This can cause undefined behaviour.
>
> Where is the double unlock of head->lock (which is a spinlock and not a
> mutex, btw)?
Maybe head is just confused with the preceding head2 as the two are
the same type of struct. They are pointers of different hash tables
though.
bind()ing two sockets to the same 2-tuple will easily trigger hung task.
>
> >
> > Signed-off-by: Abhinav Singh <[email protected]>
> > ---
> > net/ipv4/inet_connection_sock.c | 1 -
> > 1 file changed, 1 deletion(-)
> >
> > diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
> > index aeebe8816689..f11fe8c727a4 100644
> > --- a/net/ipv4/inet_connection_sock.c
> > +++ b/net/ipv4/inet_connection_sock.c
> > @@ -597,7 +597,6 @@ int inet_csk_get_port(struct sock *sk, unsigned short snum)
> > }
> > if (head2_lock_acquired)
> > spin_unlock(&head2->lock);
> > - spin_unlock_bh(&head->lock);
> > return ret;
> > }
> > EXPORT_SYMBOL_GPL(inet_csk_get_port);
On Wed, Oct 11, 2023 at 04:16:30AM +0530, Abhinav Singh wrote:
> There is a double unlock on mutex. This can cause undefined behaviour.
>
> Signed-off-by: Abhinav Singh <[email protected]>
> ---
> net/ipv4/inet_connection_sock.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
> index aeebe8816689..f11fe8c727a4 100644
> --- a/net/ipv4/inet_connection_sock.c
> +++ b/net/ipv4/inet_connection_sock.c
> @@ -597,7 +597,6 @@ int inet_csk_get_port(struct sock *sk, unsigned short snum)
> }
> if (head2_lock_acquired)
> spin_unlock(&head2->lock);
> - spin_unlock_bh(&head->lock);
How was this tested?
And where is the now-needed unlock of the head->lock?
How was this change found?
And your subject line needs a lot of work...
thanks,
greg k-h
On 10/10/2023 5:28 PM, Kuniyuki Iwashima wrote:
> From: Florian Fainelli <[email protected]>
> Date: Tue, 10 Oct 2023 15:51:13 -0700
>> On 10/10/23 15:46, Abhinav Singh wrote:
>>> There is a double unlock on mutex. This can cause undefined behaviour.
>>
>> Where is the double unlock of head->lock (which is a spinlock and not a
>> mutex, btw)?
>
> Maybe head is just confused with the preceding head2 as the two are
> the same type of struct. They are pointers of different hash tables
> though.
Suspecting that much as well, though wanted to read it from the submitter.
--
Florian
On 10/11/23 12:00, Greg KH wrote:
> On Wed, Oct 11, 2023 at 04:16:30AM +0530, Abhinav Singh wrote:
>> There is a double unlock on mutex. This can cause undefined behaviour.
>>
>> Signed-off-by: Abhinav Singh <[email protected]>
>> ---
>> net/ipv4/inet_connection_sock.c | 1 -
>> 1 file changed, 1 deletion(-)
>>
>> diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
>> index aeebe8816689..f11fe8c727a4 100644
>> --- a/net/ipv4/inet_connection_sock.c
>> +++ b/net/ipv4/inet_connection_sock.c
>> @@ -597,7 +597,6 @@ int inet_csk_get_port(struct sock *sk, unsigned short snum)
>> }
>> if (head2_lock_acquired)
>> spin_unlock(&head2->lock);
>> - spin_unlock_bh(&head->lock);
>
> How was this tested?
>
> And where is the now-needed unlock of the head->lock?
>
> How was this change found?
>
> And your subject line needs a lot of work...
>
> thanks,
>
> greg k-h
Hello, I used sparse tool and got it this warning message "warning:
context imbalance in 'inet_csk_get_port' - unexpected unlock"
Due to my over excitement of sending a good patch to kernel I didnt see
correctly and misread `head` as `head2` and thought it was double
unlocking the mutex. I m very sorry. But on a different note think we
should do a check for `head->lock` as well before unlocking. Unlocking a
non locked mutex can also trigger a undefined behaviour.
Thank you,
Abhinav Singh
On Wed, Oct 11, 2023 at 5:50 PM Abhinav Singh
<[email protected]> wrote:
>
> On 10/11/23 12:00, Greg KH wrote:
> > On Wed, Oct 11, 2023 at 04:16:30AM +0530, Abhinav Singh wrote:
> >> There is a double unlock on mutex. This can cause undefined behaviour.
> >>
> >> Signed-off-by: Abhinav Singh <[email protected]>
> >> ---
> >> net/ipv4/inet_connection_sock.c | 1 -
> >> 1 file changed, 1 deletion(-)
> >>
> >> diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
> >> index aeebe8816689..f11fe8c727a4 100644
> >> --- a/net/ipv4/inet_connection_sock.c
> >> +++ b/net/ipv4/inet_connection_sock.c
> >> @@ -597,7 +597,6 @@ int inet_csk_get_port(struct sock *sk, unsigned short snum)
> >> }
> >> if (head2_lock_acquired)
> >> spin_unlock(&head2->lock);
> >> - spin_unlock_bh(&head->lock);
> >
> > How was this tested?
> >
> > And where is the now-needed unlock of the head->lock?
> >
> > How was this change found?
> >
> > And your subject line needs a lot of work...
> >
> > thanks,
> >
> > greg k-h
> Hello, I used sparse tool and got it this warning message "warning:
> context imbalance in 'inet_csk_get_port' - unexpected unlock"
> Due to my over excitement of sending a good patch to kernel I didnt see
> correctly and misread `head` as `head2` and thought it was double
> unlocking the mutex. I m very sorry. But on a different note think we
> should do a check for `head->lock` as well before unlocking. Unlocking a
> non locked mutex can also trigger a undefined behaviour.
>
There is no undefined behavior, only sparse that might be confused a little.
I do not think we can express in sparse the fact that
inet_csk_find_open_port() acquires head->lock
(head being the return value of this function...)
The following does not help.
diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
index aeebe881668996057d1495c84eee0f0b644b7ad0..ed7b3993316cd1ba0b2859b0bd3f447e066bd3b5
100644
--- a/net/ipv4/inet_connection_sock.c
+++ b/net/ipv4/inet_connection_sock.c
@@ -323,6 +323,7 @@ static struct inet_bind_hashbucket *
inet_csk_find_open_port(const struct sock *sk, struct
inet_bind_bucket **tb_ret,
struct inet_bind2_bucket **tb2_ret,
struct inet_bind_hashbucket **head2_ret, int *port_ret)
+ __acquires(head->lock)
{
struct inet_hashinfo *hinfo = tcp_or_dccp_get_hashinfo(sk);
int i, low, high, attempt_half, port, l3mdev;
On 10/11/23 23:13, Eric Dumazet wrote:
>
> There is no undefined behavior, only sparse that might be confused a little.
>
> I do not think we can express in sparse the fact that
> inet_csk_find_open_port() acquires head->lock
>
> (head being the return value of this function...)
>
> The following does not help.
>
> diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
> index aeebe881668996057d1495c84eee0f0b644b7ad0..ed7b3993316cd1ba0b2859b0bd3f447e066bd3b5
> 100644
> --- a/net/ipv4/inet_connection_sock.c
> +++ b/net/ipv4/inet_connection_sock.c
> @@ -323,6 +323,7 @@ static struct inet_bind_hashbucket *
> inet_csk_find_open_port(const struct sock *sk, struct
> inet_bind_bucket **tb_ret,
> struct inet_bind2_bucket **tb2_ret,
> struct inet_bind_hashbucket **head2_ret, int *port_ret)
> + __acquires(head->lock)
> {
> struct inet_hashinfo *hinfo = tcp_or_dccp_get_hashinfo(sk);
> int i, low, high, attempt_half, port, l3mdev;
Okay got it. Thank you for your time maintainers.And apologizes for not
thoroughly checking before sending the patch. I have a question, there
were some type checking warning as well from sparse tool. Can a create
patch for fixing those type checking warning?
Hello,
kernel test robot noticed "INFO:rcu_sched_self-detected_stall_on_CPU" on:
commit: f0b98c2f83318c5b0668d160244c97a4c9b4f823 ("[PATCH] Remove extra unlock for the mutex")
url: https://github.com/intel-lab-lkp/linux/commits/Abhinav-Singh/Remove-extra-unlock-for-the-mutex/20231011-064851
base: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git 1c8b86a3799f7e5be903c3f49fcdaee29fd385b5
patch link: https://lore.kernel.org/all/[email protected]/
patch subject: [PATCH] Remove extra unlock for the mutex
in testcase: boot
compiler: gcc-12
test machine: qemu-system-x86_64 -enable-kvm -cpu SandyBridge -smp 2 -m 16G
(please refer to attached dmesg/kmsg for entire log/backtrace)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-lkp/[email protected]
[ 340.173373][ C0] rcu: INFO: rcu_sched self-detected stall on CPU
[ 340.174586][ C0] rcu: 0-....: (1 GPs behind) idle=03dc/1/0x4000000000000000 softirq=3450/3451 fqs=24649
[ 340.175977][ C0] rcu: (t=100002 jiffies g=3977 q=51 ncpus=2)
[ 340.176937][ C0] CPU: 0 PID: 84 Comm: (sd-listen) Not tainted 6.6.0-rc5-00056-gf0b98c2f8331 #1
[ 340.178240][ C0] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.2-debian-1.16.2-1 04/01/2014
[ 340.179747][ C0] RIP: 0010:native_queued_spin_lock_slowpath (arch/x86/include/asm/vdso/processor.h:19 arch/x86/include/asm/qspinlock.h:99 kernel/locking/qspinlock.c:327)
[ 340.180789][ C0] Code: 55 41 54 55 53 48 89 fb 66 90 ba 01 00 00 00 8b 03 85 c0 75 13 f0 0f b1 13 85 c0 75 f2 5b 5d 41 5c 41 5d c3 cc cc cc cc f3 90 <eb> e3 81 fe 00 01 00 00 74 4a 81 fe ff 00 00 00 77 77 f0 0f ba 2b
All code
========
0: 55 push %rbp
1: 41 54 push %r12
3: 55 push %rbp
4: 53 push %rbx
5: 48 89 fb mov %rdi,%rbx
8: 66 90 xchg %ax,%ax
a: ba 01 00 00 00 mov $0x1,%edx
f: 8b 03 mov (%rbx),%eax
11: 85 c0 test %eax,%eax
13: 75 13 jne 0x28
15: f0 0f b1 13 lock cmpxchg %edx,(%rbx)
19: 85 c0 test %eax,%eax
1b: 75 f2 jne 0xf
1d: 5b pop %rbx
1e: 5d pop %rbp
1f: 41 5c pop %r12
21: 41 5d pop %r13
23: c3 retq
24: cc int3
25: cc int3
26: cc int3
27: cc int3
28: f3 90 pause
2a:* eb e3 jmp 0xf <-- trapping instruction
2c: 81 fe 00 01 00 00 cmp $0x100,%esi
32: 74 4a je 0x7e
34: 81 fe ff 00 00 00 cmp $0xff,%esi
3a: 77 77 ja 0xb3
3c: f0 lock
3d: 0f .byte 0xf
3e: ba .byte 0xba
3f: 2b .byte 0x2b
Code starting with the faulting instruction
===========================================
0: eb e3 jmp 0xffffffffffffffe5
2: 81 fe 00 01 00 00 cmp $0x100,%esi
8: 74 4a je 0x54
a: 81 fe ff 00 00 00 cmp $0xff,%esi
10: 77 77 ja 0x89
12: f0 lock
13: 0f .byte 0xf
14: ba .byte 0xba
15: 2b .byte 0x2b
[ 340.183349][ C0] RSP: 0018:ffffb338c047fdc8 EFLAGS: 00000202
[ 340.184239][ C0] RAX: 0000000000000001 RBX: ffff9f0300e68d60 RCX: ffff9f03003b6bf8
[ 340.185380][ C0] RDX: 0000000000000001 RSI: 0000000000000001 RDI: ffff9f0300e68d60
[ 340.186574][ C0] RBP: 000000000000006f R08: 0000000000000000 R09: 0000000000000000
[ 340.187764][ C0] R10: 0000000000000000 R11: 0000000000000000 R12: ffffffff86d34d40
[ 340.188949][ C0] R13: 0000000000000000 R14: ffffffff86d37a80 R15: ffff9f0300e68d60
[ 340.190188][ C0] FS: 00007f34324f1900(0000) GS:ffff9f062fc00000(0000) knlGS:0000000000000000
[ 340.191487][ C0] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 340.192453][ C0] CR2: 000055e1849fc3fc CR3: 0000000100856000 CR4: 00000000000406f0
[ 340.193640][ C0] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 340.194791][ C0] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[ 340.195997][ C0] Call Trace:
[ 340.196616][ C0] <IRQ>
[ 340.197133][ C0] ? rcu_dump_cpu_stacks (kernel/rcu/tree_stall.h:372)
[ 340.197953][ C0] ? print_cpu_stall (kernel/rcu/tree_stall.h:692)
[ 340.198677][ C0] ? check_cpu_stall (kernel/rcu/tree_stall.h:775)
[ 340.199433][ C0] ? rcu_pending (kernel/rcu/tree_nocb.h:1018 kernel/rcu/tree.c:3865)
[ 340.200129][ C0] ? rcu_sched_clock_irq (kernel/rcu/tree.c:2240)
[ 340.200928][ C0] ? update_process_times (arch/x86/include/asm/preempt.h:27 kernel/time/timer.c:2073)
[ 340.201731][ C0] ? tick_sched_handle (kernel/time/tick-sched.c:255)
[ 340.202471][ C0] ? tick_sched_timer (kernel/time/tick-sched.c:1497)
[ 340.203215][ C0] ? __pfx_tick_sched_timer (kernel/time/tick-sched.c:1479)
[ 340.204056][ C0] ? __hrtimer_run_queues (kernel/time/hrtimer.c:1688 kernel/time/hrtimer.c:1752)
[ 340.204889][ C0] ? hrtimer_interrupt (kernel/time/hrtimer.c:1817)
[ 340.205654][ C0] ? __sysvec_apic_timer_interrupt (arch/x86/include/asm/jump_label.h:27 include/linux/jump_label.h:207 arch/x86/include/asm/trace/irq_vectors.h:41 arch/x86/kernel/apic/apic.c:1081)
[ 340.206540][ C0] ? sysvec_apic_timer_interrupt (arch/x86/kernel/apic/apic.c:1074 (discriminator 14))
[ 340.207374][ C0] </IRQ>
[ 340.207877][ C0] <TASK>
[ 340.208388][ C0] ? asm_sysvec_apic_timer_interrupt (arch/x86/include/asm/idtentry.h:645)
[ 340.209329][ C0] ? native_queued_spin_lock_slowpath (arch/x86/include/asm/vdso/processor.h:19 arch/x86/include/asm/qspinlock.h:99 kernel/locking/qspinlock.c:327)
[ 340.210253][ C0] _raw_spin_lock_bh (arch/x86/include/asm/paravirt.h:586 arch/x86/include/asm/qspinlock.h:51 include/asm-generic/qspinlock.h:114 include/linux/spinlock.h:187 include/linux/spinlock_api_smp.h:127 kernel/locking/spinlock.c:178)
[ 340.210996][ C0] inet_csk_get_port (net/ipv4/inet_connection_sock.c:536)
[ 340.211757][ C0] inet_csk_listen_start (net/ipv4/inet_connection_sock.c:1255)
[ 340.212570][ C0] __inet_listen_sk (net/ipv4/af_inet.c:218)
[ 340.213320][ C0] ? __cond_resched (kernel/sched/core.c:8581)
[ 340.214041][ C0] ? aa_sk_perm (security/apparmor/include/cred.h:153 security/apparmor/net.c:176)
[ 340.214778][ C0] ? __cond_resched (kernel/sched/core.c:8581)
[ 340.215488][ C0] ? lock_sock_nested (net/core/sock.c:3509)
[ 340.216234][ C0] inet_listen (net/ipv4/af_inet.c:239)
[ 340.216950][ C0] __sys_listen (include/linux/file.h:32 net/socket.c:1885)
[ 340.217686][ C0] __x64_sys_listen (net/socket.c:1892 net/socket.c:1890 net/socket.c:1890)
[ 340.218414][ C0] do_syscall_64 (arch/x86/entry/common.c:50 arch/x86/entry/common.c:80)
[ 340.219135][ C0] entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:120)
[ 340.220005][ C0] RIP: 0033:0x7f3432ccefa7
[ 340.220771][ C0] Code: f0 ff ff 73 01 c3 48 8b 0d e6 ee 0b 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 b8 32 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d b9 ee 0b 00 f7 d8 64 89 01 48
All code
========
0: f0 ff lock (bad)
2: ff 73 01 pushq 0x1(%rbx)
5: c3 retq
6: 48 8b 0d e6 ee 0b 00 mov 0xbeee6(%rip),%rcx # 0xbeef3
d: f7 d8 neg %eax
f: 64 89 01 mov %eax,%fs:(%rcx)
12: 48 83 c8 ff or $0xffffffffffffffff,%rax
16: c3 retq
17: 66 2e 0f 1f 84 00 00 nopw %cs:0x0(%rax,%rax,1)
1e: 00 00 00
21: 66 90 xchg %ax,%ax
23: b8 32 00 00 00 mov $0x32,%eax
28: 0f 05 syscall
2a:* 48 3d 01 f0 ff ff cmp $0xfffffffffffff001,%rax <-- trapping instruction
30: 73 01 jae 0x33
32: c3 retq
33: 48 8b 0d b9 ee 0b 00 mov 0xbeeb9(%rip),%rcx # 0xbeef3
3a: f7 d8 neg %eax
3c: 64 89 01 mov %eax,%fs:(%rcx)
3f: 48 rex.W
Code starting with the faulting instruction
===========================================
0: 48 3d 01 f0 ff ff cmp $0xfffffffffffff001,%rax
6: 73 01 jae 0x9
8: c3 retq
9: 48 8b 0d b9 ee 0b 00 mov 0xbeeb9(%rip),%rcx # 0xbeec9
10: f7 d8 neg %eax
12: 64 89 01 mov %eax,%fs:(%rcx)
15: 48 rex.W
The kernel config and materials to reproduce are available at:
https://download.01.org/0day-ci/archive/20231016/[email protected]
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki