From mboxrd@z Thu Jan 1 00:00:00 1970 From: Patrick McHardy Subject: Re: [PATCH 4/5] lguest: use KVM hypercalls Date: Tue, 14 Apr 2009 13:54:30 +0200 Message-ID: <49E47976.8020005@trash.net> References: <200903271022.38244.rusty@rustcorp.com.au> <1238709324.5823.8.camel@odie.local> <1239043798.27826.93.camel@zetabook> <200904081021.39877.rusty@rustcorp.com.au> <1239224319.17844.16.camel@zetabook> <49DDE91A.8060603@trash.net> <49DDF614.1060909@trash.net> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="------------020702030805060302010901" Return-path: In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: lguest-bounces+glkvl-lguest=m.gmane.org-mnsaURCQ41sdnm+yROfE0A@public.gmane.org Errors-To: lguest-bounces+glkvl-lguest=m.gmane.org-mnsaURCQ41sdnm+yROfE0A@public.gmane.org To: "Eric W. Biederman" Cc: lguest-mnsaURCQ41sdnm+yROfE0A@public.gmane.org, Herbert Xu , virtualization-qjLDD68F18O7TbgM5vRIOg@public.gmane.org, Matias Zabaljauregui List-Id: virtualization@lists.linuxfoundation.org This is a multi-part message in MIME format. --------------020702030805060302010901 Content-Type: text/plain; charset=ISO-8859-15; format=flowed Content-Transfer-Encoding: 7bit Eric W. Biederman wrote: > Patrick McHardy writes: > >> When creating the device using tunctl the sk->sk_sleep poiner is >> set to the read_wait completion of the file opened by tunctl, but >> it is not refreshed when attaching to lguest or released when >> closing the file, causing a stale pointer dereference in >> tun_sock_write_space(). >> >> Eric, please review. Thanks. > > That looks a little better. Certainly as the socket currently > lives with the tun_struct instead of the tun_file it make sense. > I'm not at all certain it makes sense for the socket to live in > tun_struct instead of tun_file. > > I happened to glance at the code about a week ago, and realized that > the introduction of the socket had done horribly things to the > guarantees I had introduced, and I haven't had a chance to think > through and figure out what the code should be doing. > > I am certain that: > opening a tap device and then "ip link del tap0" while holding > the tap open leads into a territory of madness right now. > > And apparently so does reattaching to an existing tun device. > > Patrick I'm not seeing anything in the particular patch you pointed > out that would cause crashes. It might have been a different patch or a combination, I assumed it was your patch since git annotate pointed to it and it was a very recent change. > Other lurking bugs aside your patch appears slightly off. > > tun->sk->sk_sleep in __tun_detach is correct. > > Setting sk_sleep on both paths to tun_attach instead > of in tun_attach is wrong. You are performing the assignment > before we complete the permission checks into tun_attach, which > means there is no guarantee that the tun_attach will succeed. I see. How about this patch instead? It moves the sk_sleep assignment to tun_attach, after the permission checks took place. Thanks. --------------020702030805060302010901 Content-Type: text/plain; name="x" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="x" diff --git a/drivers/net/tun.c b/drivers/net/tun.c index a1b0697..4c5ae95 100644 --- a/drivers/net/tun.c +++ b/drivers/net/tun.c @@ -155,6 +155,7 @@ static int tun_attach(struct tun_struct *tun, struct file *file) err = 0; tfile->tun = tun; tun->tfile = tfile; + tun->sk->sk_sleep = &tfile->read_wait; dev_hold(tun->dev); atomic_inc(&tfile->count); @@ -173,6 +174,8 @@ static void __tun_detach(struct tun_struct *tun) tun->tfile = NULL; netif_tx_unlock_bh(tun->dev); + tun->sk->sk_sleep = NULL; + /* Drop read queue */ skb_queue_purge(&tun->readq); @@ -861,7 +864,6 @@ static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr) struct sock *sk; struct tun_struct *tun; struct net_device *dev; - struct tun_file *tfile = file->private_data; int err; dev = __dev_get_by_name(net, ifr->ifr_name); @@ -925,7 +927,6 @@ static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr) sk->sk_write_space = tun_sock_write_space; sk->sk_destruct = tun_sock_destruct; sk->sk_sndbuf = INT_MAX; - sk->sk_sleep = &tfile->read_wait; tun->sk = sk; container_of(sk, struct tun_sock, sk)->tun = tun; --------------020702030805060302010901 Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Lguest mailing list Lguest-mnsaURCQ41sdnm+yROfE0A@public.gmane.org https://ozlabs.org/mailman/listinfo/lguest --------------020702030805060302010901--