netdev - Re: [PATCH v2] ipc/mqueue: fix potential sleeping issue in mqueue_flush

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <4a41a867-ccfc-09e1-ebaa-9f0825bd153a@huawei.com>
Date: Fri, 19 Jan 2024 09:18:35 +0800
From: shaozhengchao <shaozhengchao@...wei.com>
To: Hillf Danton <hdanton@...a.com>
CC: <linux-kernel@...r.kernel.org>, <netdev@...r.kernel.org>, Davidlohr Bueso
	<dave@...olabs.net>, Manfred Spraul <manfred@...orfullife.com>,
	<jack@...e.cz>
Subject: Re: [PATCH v2] ipc/mqueue: fix potential sleeping issue in
 mqueue_flush_file



On 2024/1/18 19:46, Hillf Danton wrote:
> On 2023/12/20 10:12, Zhengchao Shao wrote:
>> I analyze the potential sleeping issue of the following processes:
>> Thread A                                Thread B
>> ...                                     netlink_create  //ref = 1
>> do_mq_notify                            ...
>>     sock = netlink_getsockbyfilp          ...     //ref = 2
>>     info->notify_sock = sock;             ...
>> ...                                     netlink_sendmsg
>> ...                                       skb = netlink_alloc_large_skb  //skb->head is vmalloced
>> ...                                       netlink_unicast
>> ...                                         sk = netlink_getsockbyportid //ref = 3
>> ...                                         netlink_sendskb
>> ...                                           __netlink_sendskb
>> ...                                             skb_queue_tail //put skb to sk_receive_queue
>> ...                                         sock_put //ref = 2
>> ...                                     ...
>> ...                                     netlink_release
>> ...                                       deferred_put_nlk_sk //ref = 1
>> mqueue_flush_file
>>     spin_lock
>>     remove_notification
>>       netlink_sendskb
>>         sock_put  //ref = 0
>>           sk_free
>>             ...
>>             __sk_destruct
>>               netlink_sock_destruct
>>                 skb_queue_purge  //get skb from sk_receive_queue
>>                   ...
>>                   __skb_queue_purge_reason
>>                     kfree_skb_reason
>>                       __kfree_skb
>>                       ...
>>                       skb_release_all
>>                         skb_release_head_state
>>                           netlink_skb_destructor
>>                             vfree(skb->head)  //sleeping while holding spinlock
>>
>> In netlink_sendmsg, if the memory pointed to by skb->head is allocated by
>> vmalloc, and is put to sk_receive_queue queue, also the skb is not freed.
>> When the mqueue executes flush, the sleeping bug will occur. Use mutex
>> lock instead of spin lock in mqueue_flush_file.
> 
> It makes no sense to replace spinlock with mutex just for putting sock.
> 
> Only for thoughts.
> 
> --- x/ipc/mqueue.c
> +++ y/ipc/mqueue.c
> @@ -663,12 +663,17 @@ static ssize_t mqueue_read_file(struct f
>   static int mqueue_flush_file(struct file *filp, fl_owner_t id)
>   {
>   	struct mqueue_inode_info *info = MQUEUE_I(file_inode(filp));
> +	struct sock *sk = NULL;
>   
>   	spin_lock(&info->lock);
> -	if (task_tgid(current) == info->notify_owner)
> +	if (task_tgid(current) == info->notify_owner) {
> +		sk = info->notify_sock;
> +		sock_hold(sk);
>   		remove_notification(info);
> -
> +	}
>   	spin_unlock(&info->lock);
> +	if (sk)
> +		sock_put(sk);
>   	return 0;
>   }
>   

Hi Hillf:
	Thank you for your review. Your changes are more concise and
clear. I will send V3.

Zhengchao Shao