Received-SPF: pass (google.com: domain of linux-kernel+bounces-30677-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <4a41a867-ccfc-09e1-ebaa-9f0825bd153a@huawei.com>
Date: Fri, 19 Jan 2024 09:18:35 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.0.2
Subject: Re: [PATCH v2] ipc/mqueue: fix potential sleeping issue in
 mqueue_flush_file
To: Hillf Danton <hdanton@sina.com>
CC: <linux-kernel@vger.kernel.org>, <netdev@vger.kernel.org>, Davidlohr Bueso
	<dave@stgolabs.net>, Manfred Spraul <manfred@colorfullife.com>,
	<jack@suse.cz>
References: <20231220021208.2634523-1-shaozhengchao@huawei.com>
 <20240118114631.1490-1-hdanton@sina.com>
From: shaozhengchao <shaozhengchao@huawei.com>
In-Reply-To: <20240118114631.1490-1-hdanton@sina.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit


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