Re: [PATCH v2] ipc/mqueue: fix potential sleeping issue in mqueue_flush_file

[shaozhengchao]

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