netdev - Re: [PATCH v2] net-tun: restructure tun_do_read for better sleep/wakeup efficiency

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <537ADF5C.6040803@redhat.com>
Date:	Tue, 20 May 2014 12:51:40 +0800
From:	Jason Wang <jasowang@...hat.com>
To:	"Michael S. Tsirkin" <mst@...hat.com>
CC:	Xi Wang <xii@...gle.com>, "David S. Miller" <davem@...emloft.net>,
	netdev@...r.kernel.org, Maxim Krasnyansky <maxk@....qualcomm.com>,
	Neal Cardwell <ncardwell@...gle.com>,
	Eric Dumazet <edumazet@...gle.com>
Subject: Re: [PATCH v2] net-tun: restructure tun_do_read for better sleep/wakeup
 efficiency

On 05/20/2014 12:06 AM, Michael S. Tsirkin wrote:
> On Mon, May 19, 2014 at 05:27:41PM +0800, Jason Wang wrote:
>>> @@ -1330,47 +1329,26 @@ done:
>>>  static ssize_t tun_do_read(struct tun_struct *tun, struct tun_file *tfile,
>>>  			   const struct iovec *iv, ssize_t len, int noblock)
>>>  {
>>> -	DECLARE_WAITQUEUE(wait, current);
>>>  	struct sk_buff *skb;
>>>  	ssize_t ret = 0;
>>> +	int peeked, err, off = 0;
>>>  
>>>  	tun_debug(KERN_INFO, tun, "tun_do_read\n");
>>>  
>>> -	if (unlikely(!noblock))
>>> -		add_wait_queue(&tfile->wq.wait, &wait);
>>> -	while (len) {
>>> -		if (unlikely(!noblock))
>>> -			current->state = TASK_INTERRUPTIBLE;
>>> +	if (!len)
>>> +		return ret;
>>>  
>>> -		/* Read frames from the queue */
>>> -		if (!(skb = skb_dequeue(&tfile->socket.sk->sk_receive_queue))) {
>>> -			if (noblock) {
>>> -				ret = -EAGAIN;
>>> -				break;
>>> -			}
>>> -			if (signal_pending(current)) {
>>> -				ret = -ERESTARTSYS;
>>> -				break;
>>> -			}
>>> -			if (tun->dev->reg_state != NETREG_REGISTERED) {
>>> -				ret = -EIO;
>>> -				break;
>>> -			}
>>> -
>>> -			/* Nothing to read, let's sleep */
>>> -			schedule();
>>> -			continue;
>>> -		}
>>> +	if (tun->dev->reg_state != NETREG_REGISTERED)
>>> +		return -EIO;
>>>  
>>> +	/* Read frames from queue */
>>> +	skb = __skb_recv_datagram(tfile->socket.sk, noblock ? MSG_DONTWAIT : 0,
>>> +				  &peeked, &off, &err);
>>> +	if (skb) {
>> Still a little bit difference. We check the reg_state after we're sure
>> there's nothing left in sk_receive_queue. But this patch returns -EIO
>> before trying to dequeue skb.
>
> Yes but what's the concern here? What does userspace do
> to notice the change in behaviour?
> tun_detach calls tun_queue_purge before unregister_netdevice
> so apparently there's never anything in queue when
> state isn't registered.
> Did I miss something?
>

>From rollback_registered_many(), ndo_uninit() was called after reg_state
was changed to NETREG_UNREGISTERING. So userspace could read something
in the small window. Not sure it was a problem, but it does have minor
difference.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html