[PATCH] Remove extra unlock for the mutex

[PATCH] Remove extra unlock for the mutex

[Abhinav Singh]

There is a double unlock on mutex. This can cause undefined behaviour.

Signed-off-by: Abhinav Singh <singhabhinav9051571833@gmail.com>
---
 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

Re: [PATCH] Remove extra unlock for the mutex

[Florian Fainelli]

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 <singhabhinav9051571833@gmail.com>
> ---
>   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

Re: [PATCH] Remove extra unlock for the mutex

[Kuniyuki Iwashima]

From: Florian Fainelli <f.fainelli@gmail.com>
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 <singhabhinav9051571833@gmail.com>
> > ---
> >   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);

Re: [PATCH] Remove extra unlock for the mutex

[Florian Fainelli]

On 10/10/2023 5:28 PM, Kuniyuki Iwashima wrote:
> From: Florian Fainelli <f.fainelli@gmail.com>
> 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

Re: [PATCH] Remove extra unlock for the mutex

[Greg KH]

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 <singhabhinav9051571833@gmail.com>
> ---
>  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

Re: [PATCH] Remove extra unlock for the mutex

[Abhinav Singh]

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 <singhabhinav9051571833@gmail.com>
>> ---
>>   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

Re: [PATCH] Remove extra unlock for the mutex

[Eric Dumazet]

On Wed, Oct 11, 2023 at 5:50 PM Abhinav Singh
<singhabhinav9051571833@gmail.com> 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 <singhabhinav9051571833@gmail.com>
> >> ---
> >>   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;

Re: [PATCH] Remove extra unlock for the mutex

[Abhinav Singh]

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?

Re: [PATCH] Remove extra unlock for the mutex

[kernel test robot]

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/20231010224630.238254-1-singhabhinav9051571833@gmail.com/
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 <oliver.sang@intel.com>
| Closes: https://lore.kernel.org/oe-lkp/202310162034.6677994c-oliver.sang@intel.com


[  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/202310162034.6677994c-oliver.sang@intel.com



-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki