[RFC PATCH v1] tun: Cleanup error handling in tun_set_iff()

[RFC PATCH v1] tun: Cleanup error handling in tun_set_iff()

[Paul Moore]

Convert some pointless gotos into returns and ensure tun_attach() errors are
handled correctly.

Signed-off-by: Paul Moore <paul.moore@hp.com>

--

I'm sending this as an RFC patch because I'm not entirely certain that the
changes to the tun_attach() error handling code are 100% correct, although I
strongly suspect that the current behavor of simply returning an error code
without any cleanup is broken.  Can anyone comment on this?
---

 drivers/net/tun.c |   19 ++++++++++---------
 1 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 4a0db7a..b6d06fd 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -938,13 +938,10 @@ static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr)
 		err = tun_attach(tun, file);
 		if (err < 0)
 			return err;
-	}
-	else {
+	} else {
 		char *name;
 		unsigned long flags = 0;
 
-		err = -EINVAL;
-
 		if (!capable(CAP_NET_ADMIN))
 			return -EPERM;
 
@@ -958,7 +955,7 @@ static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr)
 			flags |= TUN_TAP_DEV;
 			name = "tap%d";
 		} else
-			goto failed;
+			return -EINVAL;
 
 		if (*ifr->ifr_name)
 			name = ifr->ifr_name;
@@ -976,10 +973,11 @@ static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr)
 		tun->flags = flags;
 		tun->txflt.count = 0;
 
-		err = -ENOMEM;
 		sk = sk_alloc(net, AF_UNSPEC, GFP_KERNEL, &tun_proto);
-		if (!sk)
+		if (!sk) {
+			err = -ENOMEM;
 			goto err_free_dev;
+		}
 
 		init_waitqueue_head(&tun->socket.wait);
 		sock_init_data(&tun->socket, sk);
@@ -1010,7 +1008,7 @@ static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr)
 
 		err = tun_attach(tun, file);
 		if (err < 0)
-			goto failed;
+			goto err_unreg_dev;
 	}
 
 	DBG(KERN_INFO "%s: tun_set_iff\n", tun->dev->name);
@@ -1039,11 +1037,14 @@ static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr)
 	strcpy(ifr->ifr_name, tun->dev->name);
 	return 0;
 
+ err_unreg_dev:
+	rtnl_lock();
+	unregister_netdevice(tun->dev);
+	rtnl_unlock();
  err_free_sk:
 	sock_put(sk);
  err_free_dev:
 	free_netdev(dev);
- failed:
 	return err;
 }
 

Re: [RFC PATCH v1] tun: Cleanup error handling in tun_set_iff()

[David Miller]

From: Paul Moore <paul.moore@hp.com>
Date: Mon, 03 Aug 2009 12:12:43 -0400

> Convert some pointless gotos into returns and ensure tun_attach() errors are
> handled correctly.
> 
> Signed-off-by: Paul Moore <paul.moore@hp.com>
> 
> --
> 
> I'm sending this as an RFC patch because I'm not entirely certain that the
> changes to the tun_attach() error handling code are 100% correct, although I
> strongly suspect that the current behavor of simply returning an error code
> without any cleanup is broken.  Can anyone comment on this?

It looks good to me.

But I've been wrong about this code before, so it would be nice
to get some other eyes on it :-)

Re: [RFC PATCH v1] tun: Cleanup error handling in tun_set_iff()

[Eric W. Biederman]

Paul Moore <paul.moore@hp.com> writes:

> Convert some pointless gotos into returns and ensure tun_attach() errors are
> handled correctly.
>
> Signed-off-by: Paul Moore <paul.moore@hp.com>

This patch appears slightly wrong.  Comments inline.

> I'm sending this as an RFC patch because I'm not entirely certain that the
> changes to the tun_attach() error handling code are 100% correct, although I
> strongly suspect that the current behavor of simply returning an error code
> without any cleanup is broken.  Can anyone comment on this?
> ---
>
>  drivers/net/tun.c |   19 ++++++++++---------
>  1 files changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index 4a0db7a..b6d06fd 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -938,13 +938,10 @@ static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr)
>  		err = tun_attach(tun, file);
>  		if (err < 0)
>  			return err;
> -	}
> -	else {
> +	} else {
>  		char *name;
>  		unsigned long flags = 0;
>  
> -		err = -EINVAL;
> -
>  		if (!capable(CAP_NET_ADMIN))
>  			return -EPERM;
>  
> @@ -958,7 +955,7 @@ static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr)
>  			flags |= TUN_TAP_DEV;
>  			name = "tap%d";
>  		} else
> -			goto failed;
> +			return -EINVAL;

Trivially correct.

>  
>  		if (*ifr->ifr_name)
>  			name = ifr->ifr_name;
> @@ -976,10 +973,11 @@ static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr)
>  		tun->flags = flags;
>  		tun->txflt.count = 0;
>  
> -		err = -ENOMEM;
>  		sk = sk_alloc(net, AF_UNSPEC, GFP_KERNEL, &tun_proto);
> -		if (!sk)
> +		if (!sk) {
> +			err = -ENOMEM;
>  			goto err_free_dev;
> +		}

Trivially correct but I would argue uglier.

>  
>  		init_waitqueue_head(&tun->socket.wait);
>  		sock_init_data(&tun->socket, sk);
> @@ -1010,7 +1008,7 @@ static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr)
>  
>  		err = tun_attach(tun, file);
>  		if (err < 0)
> -			goto failed;
> +			goto err_unreg_dev;
>  	}

This is the interesting one.  And several problems with it.  When
Herbert reworked the reference counting here he introduced the goto
failed; Instead of err_free_dev.

I think there were more possible races when I introduced the check
of the return code of tun_attach, the only case I can see currently
is if the file was attached to another tun device before we grabbed the
rtnl_lock.


>  	DBG(KERN_INFO "%s: tun_set_iff\n", tun->dev->name);
> @@ -1039,11 +1037,14 @@ static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr)
>  	strcpy(ifr->ifr_name, tun->dev->name);
>  	return 0;
>  
> + err_unreg_dev:
> +	rtnl_lock();
> +	unregister_netdevice(tun->dev);
> +	rtnl_unlock();

This is a guaranteed deadlock.  We already hold the rtnl_lock here.
Furthermore all I should need to do in this case is to call
unregister_netdevice(...).    As all of the rest of the pieces should
happen in the cleanup routines called from unregister_netdevice.

The current behavior of returning an error code is a little bit off
as it creates a persistent tun device without setting tun->flags | TUN_PERSIST.
And it leaves a tun device for someone else to clean up.

At the same time it should be perfectly legitimate for someone else to
come along and attach to the tun device and delete it or to call
ip link del <tun>

Eric

>   err_free_sk:
>  	sock_put(sk);
>   err_free_dev:
>  	free_netdev(dev);
> - failed:
>  	return err;
>  }
>  
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v1] tun: Cleanup error handling in tun_set_iff()

[Paul Moore]

On Wednesday 05 August 2009 01:32:49 am Eric W. Biederman wrote:
> Paul Moore <paul.moore@hp.com> writes:
> > Convert some pointless gotos into returns and ensure tun_attach() errors
> > are handled correctly.
> >
> > Signed-off-by: Paul Moore <paul.moore@hp.com>
>
> This patch appears slightly wrong.  Comments inline.

Thanks for taking a look ...

> > I'm sending this as an RFC patch because I'm not entirely certain that
> > the changes to the tun_attach() error handling code are 100% correct,
> > although I strongly suspect that the current behavor of simply returning
> > an error code without any cleanup is broken.  Can anyone comment on this?
> > ---
> >
> >  drivers/net/tun.c |   19 ++++++++++---------
> >  1 files changed, 10 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> > index 4a0db7a..b6d06fd 100644
> > --- a/drivers/net/tun.c
> > +++ b/drivers/net/tun.c
> > @@ -976,10 +973,11 @@ static int tun_set_iff(struct net *net, struct file
> > *file, struct ifreq *ifr) tun->flags = flags;
> >  		tun->txflt.count = 0;
> >
> > -		err = -ENOMEM;
> >  		sk = sk_alloc(net, AF_UNSPEC, GFP_KERNEL, &tun_proto);
> > -		if (!sk)
> > +		if (!sk) {
> > +			err = -ENOMEM;
> >  			goto err_free_dev;
> > +		}
>
> Trivially correct but I would argue uglier.

My reasoning behind the change was that code related to the error handling 
should be moved outside the common path as much as possible similar to what we 
do now with the gotos.

> > @@ -1010,7 +1008,7 @@ static int tun_set_iff(struct net *net, struct file
> > *file, struct ifreq *ifr)
> >
> >  		err = tun_attach(tun, file);
> >  		if (err < 0)
> > -			goto failed;
> > +			goto err_unreg_dev;
> >  	}
>
> This is the interesting one.  And several problems with it.  When
> Herbert reworked the reference counting here he introduced the goto
> failed; Instead of err_free_dev.
>
> I think there were more possible races when I introduced the check
> of the return code of tun_attach, the only case I can see currently
> is if the file was attached to another tun device before we grabbed the
> rtnl_lock.

...

> > @@ -1039,11 +1037,14 @@ static int tun_set_iff(struct net *net, struct
> > file *file, struct ifreq *ifr) strcpy(ifr->ifr_name, tun->dev->name);
> >  	return 0;
> >
> > + err_unreg_dev:
> > +	rtnl_lock();
> > +	unregister_netdevice(tun->dev);
> > +	rtnl_unlock();
>
> This is a guaranteed deadlock.  We already hold the rtnl_lock here.
> Furthermore all I should need to do in this case is to call
> unregister_netdevice(...).    As all of the rest of the pieces should
> happen in the cleanup routines called from unregister_netdevice.

Okay, so at the very least the rtnl_[un]lock() calls need to be removed and we 
can safely return after calling unregister_netdevice() without having to 
free/release anything else.  The thing that concerns me the most are the 
potential races you talked about.

I'll admit there are many dark places here that I still don't understand but 
looking at the code it appears that the only way to get into tun_set_iff() is 
if the file is not currently attached to a TUN device which is further checked 
in tun_attach().  Also, I'm not sure what refcounting you are talking about - 
do you mean the tun_file->count refcount?  I think we should be okay in this 
regard as the only way we would end up changing tun_file->count is if 
tun_attach() completed successfully; if tun_attach() fails there is not change 
in the refcount.  I could be way off here but it _seems_ safe ... :)

> The current behavior of returning an error code is a little bit off
> as it creates a persistent tun device without setting tun->flags |
> TUN_PERSIST. And it leaves a tun device for someone else to clean up.
>
> At the same time it should be perfectly legitimate for someone else to
> come along and attach to the tun device and delete it or to call
> ip link del <tun>

My concern is that I believe that if the kernel fails at an operation it 
should do all it can to unwind any actions it took in the course of attempting 
to perform the requested operation.  Leaving a TUN device around in the case 
of a tun_attach() failure seems like the kernel is being lazy, sure a user can 
cleanup the device but why should they have to?

-- 
paul moore
linux @ hp

Re: [RFC PATCH v1] tun: Cleanup error handling in tun_set_iff()

[Eric W. Biederman]

Paul Moore <paul.moore@hp.com> writes:

> On Wednesday 05 August 2009 01:32:49 am Eric W. Biederman wrote:
>> Paul Moore <paul.moore@hp.com> writes:
>> > Convert some pointless gotos into returns and ensure tun_attach() errors
>> > are handled correctly.
>> >
>> > Signed-off-by: Paul Moore <paul.moore@hp.com>
>>
>> This patch appears slightly wrong.  Comments inline.
>
> Thanks for taking a look ...
>
>> > I'm sending this as an RFC patch because I'm not entirely certain that
>> > the changes to the tun_attach() error handling code are 100% correct,
>> > although I strongly suspect that the current behavor of simply returning
>> > an error code without any cleanup is broken.  Can anyone comment on this?
>> > ---
>> >
>> >  drivers/net/tun.c |   19 ++++++++++---------
>> >  1 files changed, 10 insertions(+), 9 deletions(-)
>> >
>> > diff --git a/drivers/net/tun.c b/drivers/net/tun.c
>> > index 4a0db7a..b6d06fd 100644
>> > --- a/drivers/net/tun.c
>> > +++ b/drivers/net/tun.c
>> > @@ -976,10 +973,11 @@ static int tun_set_iff(struct net *net, struct file
>> > *file, struct ifreq *ifr) tun->flags = flags;
>> >  		tun->txflt.count = 0;
>> >
>> > -		err = -ENOMEM;
>> >  		sk = sk_alloc(net, AF_UNSPEC, GFP_KERNEL, &tun_proto);
>> > -		if (!sk)
>> > +		if (!sk) {
>> > +			err = -ENOMEM;
>> >  			goto err_free_dev;
>> > +		}
>>
>> Trivially correct but I would argue uglier.
>
> My reasoning behind the change was that code related to the error handling 
> should be moved outside the common path as much as possible similar to what we 
> do now with the gotos.

I don't understand.  Generating less readable and less efficient code is
preferable?

>> > @@ -1010,7 +1008,7 @@ static int tun_set_iff(struct net *net, struct file
>> > *file, struct ifreq *ifr)
>> >
>> >  		err = tun_attach(tun, file);
>> >  		if (err < 0)
>> > -			goto failed;
>> > +			goto err_unreg_dev;
>> >  	}
>>
>> This is the interesting one.  And several problems with it.  When
>> Herbert reworked the reference counting here he introduced the goto
>> failed; Instead of err_free_dev.
>>
>> I think there were more possible races when I introduced the check
>> of the return code of tun_attach, the only case I can see currently
>> is if the file was attached to another tun device before we grabbed the
>> rtnl_lock.
>
> ...
>
>> > @@ -1039,11 +1037,14 @@ static int tun_set_iff(struct net *net, struct
>> > file *file, struct ifreq *ifr) strcpy(ifr->ifr_name, tun->dev->name);
>> >  	return 0;
>> >
>> > + err_unreg_dev:
>> > +	rtnl_lock();
>> > +	unregister_netdevice(tun->dev);
>> > +	rtnl_unlock();
>>
>> This is a guaranteed deadlock.  We already hold the rtnl_lock here.
>> Furthermore all I should need to do in this case is to call
>> unregister_netdevice(...).    As all of the rest of the pieces should
>> happen in the cleanup routines called from unregister_netdevice.
>
> Okay, so at the very least the rtnl_[un]lock() calls need to be removed and we 
> can safely return after calling unregister_netdevice() without having to 
> free/release anything else.  The thing that concerns me the most are the 
> potential races you talked about.
>
> I'll admit there are many dark places here that I still don't understand but 
> looking at the code it appears that the only way to get into tun_set_iff() is 
> if the file is not currently attached to a TUN device which is further checked 
> in tun_attach().  

Yes.  But how many times can you come into tun_set_iff on the same file?
Think multiple threads trying to do the same thing at the same time.

> Also, I'm not sure what refcounting you are talking about - 
> do you mean the tun_file->count refcount?  I think we should be okay in this 
> regard as the only way we would end up changing tun_file->count is if 
> tun_attach() completed successfully; if tun_attach() fails there is not change 
> in the refcount.  I could be way off here but it _seems_ safe ... :)

I totally agree that things are not as obvious as they should be.  I made
things more complex when I added support for ip link del.  Then Herbert's
patches collided and broke things when he added the socket support.
Herbert change the refcount scheme that made things simpler and easier
to get correct.

Things are mostly good now, but think the tun code could use good audit,
cleanup, simplification.  Which takes advantage of everything that Herbert
did.  All of which requires someone to spend enough time looking at the
code to understand what is going on.  Not hard but a bit time consuming.

>> The current behavior of returning an error code is a little bit off
>> as it creates a persistent tun device without setting tun->flags |
>> TUN_PERSIST. And it leaves a tun device for someone else to clean up.
>>
>> At the same time it should be perfectly legitimate for someone else to
>> come along and attach to the tun device and delete it or to call
>> ip link del <tun>
>
> My concern is that I believe that if the kernel fails at an operation it 
> should do all it can to unwind any actions it took in the course of attempting 
> to perform the requested operation.  Leaving a TUN device around in the case 
> of a tun_attach() failure seems like the kernel is being lazy, sure a user can 
> cleanup the device but why should they have to?

Sure.  I am all for that and that is how I originally coded it.
My point is that this is not harmful and that it can really only be triggered
by a buggy or hostile user.   So it isn't critical to get fixed.  It is an
imperfect so a fix is desirable.

Eric

Re: [RFC PATCH v1] tun: Cleanup error handling in tun_set_iff()

[Herbert Xu]

On Wed, Aug 05, 2009 at 05:38:42PM -0400, Paul Moore wrote:
>
> My concern is that I believe that if the kernel fails at an operation it 
> should do all it can to unwind any actions it took in the course of attempting 
> to perform the requested operation.  Leaving a TUN device around in the case 
> of a tun_attach() failure seems like the kernel is being lazy, sure a user can 
> cleanup the device but why should they have to?

That particular tun_attach should never fail.  Perhaps you can
just add a WARN_ON.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [RFC PATCH v1] tun: Cleanup error handling in tun_set_iff()

[Eric W. Biederman]

Herbert Xu <herbert@gondor.apana.org.au> writes:

> On Wed, Aug 05, 2009 at 05:38:42PM -0400, Paul Moore wrote:
>>
>> My concern is that I believe that if the kernel fails at an operation it 
>> should do all it can to unwind any actions it took in the course of attempting 
>> to perform the requested operation.  Leaving a TUN device around in the case 
>> of a tun_attach() failure seems like the kernel is being lazy, sure a user can 
>> cleanup the device but why should they have to?
>
> That particular tun_attach should never fail.  Perhaps you can
> just add a WARN_ON.

Two threads one file descriptor.  Both simultaneously attempt to 
attach to a tun device.  One will fail, the other succeed.

At least that is how I read the locking.

Eric

Re: [RFC PATCH v1] tun: Cleanup error handling in tun_set_iff()

[Herbert Xu]

On Thu, Aug 06, 2009 at 03:21:41AM -0700, Eric W. Biederman wrote:
>
> Two threads one file descriptor.  Both simultaneously attempt to 
> attach to a tun device.  One will fail, the other succeed.
> 
> At least that is how I read the locking.

Yes but the "race" fixed by this patch is centred on the tun_attach
call for a newly created network device.  As tun_set_iff occurs
under RTNL, the second thread cannot start attaching until the
creation thread has completed.  IOW the thread that creates the
net device should always succeed in attaching.

If two threads try to attach to the same device that was already
created then yes one will fail and the other succeed.  However,
AFAICS that case has nothing to do with this patch.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [RFC PATCH v1] tun: Cleanup error handling in tun_set_iff()

[Eric W. Biederman]

Herbert Xu <herbert@gondor.apana.org.au> writes:

> On Thu, Aug 06, 2009 at 03:21:41AM -0700, Eric W. Biederman wrote:
>>
>> Two threads one file descriptor.  Both simultaneously attempt to 
>> attach to a tun device.  One will fail, the other succeed.
>> 
>> At least that is how I read the locking.
>
> Yes but the "race" fixed by this patch is centred on the tun_attach
> call for a newly created network device.  As tun_set_iff occurs
> under RTNL, the second thread cannot start attaching until the
> creation thread has completed.  IOW the thread that creates the
> net device should always succeed in attaching.

>
> If two threads try to attach to the same device that was already
> created then yes one will fail and the other succeed.  However,
> AFAICS that case has nothing to do with this patch.

Summarizing:

tun = __tun_get(tfile);
if (!tun) { // No tun we are not attached.
	 < -------------------- race opportunity
	rtnl_lock();
        tun_set_iff();
        rtnl_unlock();
}
...

We don't test if we are attached under the rtnl
until we get to tun_attach();

So two threads can both do:

tun = __tun_get(tfile);
if (!tun) {
	rtnl_lock();
        tun_set_iff();
            dev = __dev_get_by_name(net, "not_an_interface_name");
            if (!dev) {
               dev = alloc_netdev(....);
               ...;
               register_netdev(dev);
               ...;
               err = tun_attach(..);
            }


Only one thread is in tun_set_iff() at a time, but the other thread
could have attached the file to a device before the one in tun_attach().

Eric

Re: [RFC PATCH v1] tun: Cleanup error handling in tun_set_iff()

[Herbert Xu]

On Thu, Aug 06, 2009 at 07:27:13AM -0700, Eric W. Biederman wrote:
>
> Summarizing:
> 
> tun = __tun_get(tfile);
> if (!tun) { // No tun we are not attached.
> 	 < -------------------- race opportunity
> 	rtnl_lock();
>         tun_set_iff();
>         rtnl_unlock();
> }
> ...
> 
> We don't test if we are attached under the rtnl
> until we get to tun_attach();
> 
> So two threads can both do:
> 
> tun = __tun_get(tfile);
> if (!tun) {
> 	rtnl_lock();
>         tun_set_iff();
>             dev = __dev_get_by_name(net, "not_an_interface_name");
>             if (!dev) {
>                dev = alloc_netdev(....);
>                ...;
>                register_netdev(dev);
>                ...;
>                err = tun_attach(..);
>             }
> 
> 
> Only one thread is in tun_set_iff() at a time, but the other thread
> could have attached the file to a device before the one in tun_attach().

Right, I see what you mean.  However I don't think this is possible
because the ioctl runs under the big kernel lock.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [RFC PATCH v1] tun: Cleanup error handling in tun_set_iff()

[Eric W. Biederman]

Herbert Xu <herbert@gondor.apana.org.au> writes:

> On Thu, Aug 06, 2009 at 07:27:13AM -0700, Eric W. Biederman wrote:
>>
>> Summarizing:
>> 
>> tun = __tun_get(tfile);
>> if (!tun) { // No tun we are not attached.
>> 	 < -------------------- race opportunity
>> 	rtnl_lock();
>>         tun_set_iff();
>>         rtnl_unlock();
>> }
>> ...
>> 
>> We don't test if we are attached under the rtnl
>> until we get to tun_attach();
>> 
>> So two threads can both do:
>> 
>> tun = __tun_get(tfile);
>> if (!tun) {
>> 	rtnl_lock();
>>         tun_set_iff();
>>             dev = __dev_get_by_name(net, "not_an_interface_name");
>>             if (!dev) {
>>                dev = alloc_netdev(....);
>>                ...;
>>                register_netdev(dev);
>>                ...;
>>                err = tun_attach(..);
>>             }
>> 
>> 
>> Only one thread is in tun_set_iff() at a time, but the other thread
>> could have attached the file to a device before the one in tun_attach().
>
> Right, I see what you mean.  However I don't think this is possible
> because the ioctl runs under the big kernel lock.

Why not?  We can sleep on that code path.
Although now that you mention it we should use unlocked_ioctl unless
we actually need the BKL.

Eric

Re: [RFC PATCH v1] tun: Cleanup error handling in tun_set_iff()

[Paul Moore]

On Thursday 06 August 2009 11:02:22 am Eric W. Biederman wrote:
> Herbert Xu <herbert@gondor.apana.org.au> writes:
> > On Thu, Aug 06, 2009 at 07:27:13AM -0700, Eric W. Biederman wrote:
> >> Summarizing:
> >>
> >> tun = __tun_get(tfile);
> >> if (!tun) { // No tun we are not attached.
> >> 	 < -------------------- race opportunity
> >> 	rtnl_lock();
> >>         tun_set_iff();
> >>         rtnl_unlock();
> >> }
> >> ...
> >>
> >> We don't test if we are attached under the rtnl
> >> until we get to tun_attach();
> >>
> >> So two threads can both do:
> >>
> >> tun = __tun_get(tfile);
> >> if (!tun) {
> >> 	rtnl_lock();
> >>         tun_set_iff();
> >>             dev = __dev_get_by_name(net, "not_an_interface_name");
> >>             if (!dev) {
> >>                dev = alloc_netdev(....);
> >>                ...;
> >>                register_netdev(dev);
> >>                ...;
> >>                err = tun_attach(..);
> >>             }
> >>
> >>
> >> Only one thread is in tun_set_iff() at a time, but the other thread
> >> could have attached the file to a device before the one in tun_attach().
> >
> > Right, I see what you mean.  However I don't think this is possible
> > because the ioctl runs under the big kernel lock.
>
> Why not?  We can sleep on that code path.
> Although now that you mention it we should use unlocked_ioctl unless
> we actually need the BKL.

Dave, if you haven't already, it is probably a good idea to just forget about 
this patch.  Prior to this discussion I suspected that the TUN driver could 
use a closer look, after reading the comments from Eric and Herbert there 
isn't much suspicion left.  I'll put this on my rainy day todo list to try and 
tackle but I won't be upset if somebody beats me to it.

-- 
paul moore
linux @ hp

Re: [RFC PATCH v1] tun: Cleanup error handling in tun_set_iff()

[Paul Moore]

On Wednesday 05 August 2009 07:14:06 pm Eric W. Biederman wrote:
> Paul Moore <paul.moore@hp.com> writes:
> > On Wednesday 05 August 2009 01:32:49 am Eric W. Biederman wrote:
> >> Paul Moore <paul.moore@hp.com> writes:
> >> > diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> >> > index 4a0db7a..b6d06fd 100644
> >> > --- a/drivers/net/tun.c
> >> > +++ b/drivers/net/tun.c
> >> > @@ -976,10 +973,11 @@ static int tun_set_iff(struct net *net, struct
> >> > file *file, struct ifreq *ifr) tun->flags = flags;
> >> >  		tun->txflt.count = 0;
> >> >
> >> > -		err = -ENOMEM;
> >> >  		sk = sk_alloc(net, AF_UNSPEC, GFP_KERNEL, &tun_proto);
> >> > -		if (!sk)
> >> > +		if (!sk) {
> >> > +			err = -ENOMEM;
> >> >  			goto err_free_dev;
> >> > +		}
> >>
> >> Trivially correct but I would argue uglier.
> >
> > My reasoning behind the change was that code related to the error
> > handling should be moved outside the common path as much as possible
> > similar to what we do now with the gotos.
>
> I don't understand.  Generating less readable and less efficient code is
> preferable?

While we can probably debate the "readability" of code all day long and get no 
where (anyone care to argue about the color of the bikeshed?) the concept of 
code efficiency should be a bit easier to quantify.  I'll admit that I'm far 
from a performance expert but here is my reasoning ...

The code currently looks something like this:

	err = -ENOMEM;
	buf = alloc(...);
	if (!buf)
		goto label;

This means that in the common case where 'alloc()' completes without error we 
are doing an extra, unnecessary assignment where we set the value in 'err'.  
Now, if we change this slightly to match what I proposed in the patch:

	buf = alloc(...);
	if (!buf) {
		err = -ENOMEM;
		goto label;
	}

We eliminate that extra assignment in the case where 'alloc()' completes 
without error, which should result in more efficient code (less instructions 
in the common case).  Am I wrong?  If that is the case I would appreciate an 
explanation ...

-- 
paul moore
linux @ hp

Re: [RFC PATCH v1] tun: Cleanup error handling in tun_set_iff()

[David Miller]

From: Paul Moore <paul.moore@hp.com>
Date: Thu, 6 Aug 2009 14:09:19 -0400

> Dave, if you haven't already, it is probably a good idea to just
> forget about this patch.

ok.

Re: [RFC PATCH v1] tun: Cleanup error handling in tun_set_iff()

[Herbert Xu]

On Thu, Aug 06, 2009 at 02:20:20PM -0400, Paul Moore wrote:
>
> The code currently looks something like this:
> 
> 	err = -ENOMEM;
> 	buf = alloc(...);
> 	if (!buf)
> 		goto label;
> 
> This means that in the common case where 'alloc()' completes without error we 
> are doing an extra, unnecessary assignment where we set the value in 'err'.  
> Now, if we change this slightly to match what I proposed in the patch:
> 
> 	buf = alloc(...);
> 	if (!buf) {
> 		err = -ENOMEM;
> 		goto label;
> 	}
> 
> We eliminate that extra assignment in the case where 'alloc()' completes 
> without error, which should result in more efficient code (less instructions 
> in the common case).  Am I wrong?  If that is the case I would appreciate an 
> explanation ...

Your style potentially introduces a second jump which may end
up being worse compared to the extra work on a modern CPU.


Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [RFC PATCH v1] tun: Cleanup error handling in tun_set_iff()

[Herbert Xu]

On Thu, Aug 06, 2009 at 08:02:22AM -0700, Eric W. Biederman wrote:
> 
> Why not?  We can sleep on that code path.
> Although now that you mention it we should use unlocked_ioctl unless
> we actually need the BKL.

You're right of course.  So the race is real, but I think there
is no good reason for such parallel operations to be allowed in
the first place.  So how about extending the coverage of the
locked section to the whole ioctl, like this:

tun: Extend RTNL lock coverage over whole ioctl

As it is, parts of the ioctl runs under the RTNL and parts of
it do not.  The unlocked section is still protected by the BKL,
but there can be subtle races.  For example, Eric Biederman and
Paul Moore observed that if two threads tried to create two tun
devices on the same file descriptor, then unexpected results
may occur.

As there isn't anything in the ioctl that is expected to sleep
indefinitely, we can prevent this from occurring by extending
the RTNL lock coverage.

This also allows to get rid of the BKL.

Finally, I changed tun_get_iff to take a tun device in order to
avoid calling tun_put which would dead-lock as it also tries to
take the RTNL lock.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 027f7ab..42b6c63 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -1048,20 +1048,15 @@ static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr)
 	return err;
 }
 
-static int tun_get_iff(struct net *net, struct file *file, struct ifreq *ifr)
+static int tun_get_iff(struct net *net, struct tun_struct *tun,
+		       struct ifreq *ifr)
 {
-	struct tun_struct *tun = tun_get(file);
-
-	if (!tun)
-		return -EBADFD;
-
 	DBG(KERN_INFO "%s: tun_get_iff\n", tun->dev->name);
 
 	strcpy(ifr->ifr_name, tun->dev->name);
 
 	ifr->ifr_flags = tun_flags(tun);
 
-	tun_put(tun);
 	return 0;
 }
 
@@ -1105,8 +1100,8 @@ static int set_offload(struct net_device *dev, unsigned long arg)
 	return 0;
 }
 
-static int tun_chr_ioctl(struct inode *inode, struct file *file,
-			 unsigned int cmd, unsigned long arg)
+static long tun_chr_ioctl(struct file *file, unsigned int cmd,
+			  unsigned long arg)
 {
 	struct tun_file *tfile = file->private_data;
 	struct tun_struct *tun;
@@ -1128,34 +1123,32 @@ static int tun_chr_ioctl(struct inode *inode, struct file *file,
 				(unsigned int __user*)argp);
 	}
 
+	rtnl_lock();
+
 	tun = __tun_get(tfile);
 	if (cmd == TUNSETIFF && !tun) {
-		int err;
-
 		ifr.ifr_name[IFNAMSIZ-1] = '\0';
 
-		rtnl_lock();
-		err = tun_set_iff(tfile->net, file, &ifr);
-		rtnl_unlock();
+		ret = tun_set_iff(tfile->net, file, &ifr);
 
-		if (err)
-			return err;
+		if (ret)
+			goto unlock;
 
 		if (copy_to_user(argp, &ifr, sizeof(ifr)))
-			return -EFAULT;
-		return 0;
+			ret = -EFAULT;
+		goto unlock;
 	}
 
-
+	ret = -EBADFD;
 	if (!tun)
-		return -EBADFD;
+		goto unlock;
 
 	DBG(KERN_INFO "%s: tun_chr_ioctl cmd %d\n", tun->dev->name, cmd);
 
 	ret = 0;
 	switch (cmd) {
 	case TUNGETIFF:
-		ret = tun_get_iff(current->nsproxy->net_ns, file, &ifr);
+		ret = tun_get_iff(current->nsproxy->net_ns, tun, &ifr);
 		if (ret)
 			break;
 
@@ -1201,7 +1194,6 @@ static int tun_chr_ioctl(struct inode *inode, struct file *file,
 
 	case TUNSETLINK:
 		/* Only allow setting the type when the interface is down */
-		rtnl_lock();
 		if (tun->dev->flags & IFF_UP) {
 			DBG(KERN_INFO "%s: Linktype set failed because interface is up\n",
 				tun->dev->name);
@@ -1211,7 +1203,6 @@ static int tun_chr_ioctl(struct inode *inode, struct file *file,
 			DBG(KERN_INFO "%s: linktype set to %d\n", tun->dev->name, tun->dev->type);
 			ret = 0;
 		}
-		rtnl_unlock();
 		break;
 
 #ifdef TUN_DEBUG
@@ -1220,9 +1211,7 @@ static int tun_chr_ioctl(struct inode *inode, struct file *file,
 		break;
 #endif
 	case TUNSETOFFLOAD:
-		rtnl_lock();
 		ret = set_offload(tun->dev, arg);
-		rtnl_unlock();
 		break;
 
 	case TUNSETTXFILTER:
@@ -1230,9 +1219,7 @@ static int tun_chr_ioctl(struct inode *inode, struct file *file,
 		ret = -EINVAL;
 		if ((tun->flags & TUN_TYPE_MASK) != TUN_TAP_DEV)
 			break;
-		rtnl_lock();
 		ret = update_filter(&tun->txflt, (void __user *)arg);
-		rtnl_unlock();
 		break;
 
 	case SIOCGIFHWADDR:
@@ -1248,9 +1235,7 @@ static int tun_chr_ioctl(struct inode *inode, struct file *file,
 		DBG(KERN_DEBUG "%s: set hw address: %pM\n",
 			tun->dev->name, ifr.ifr_hwaddr.sa_data);
 
-		rtnl_lock();
 		ret = dev_set_mac_address(tun->dev, &ifr.ifr_hwaddr);
-		rtnl_unlock();
 		break;
 
 	case TUNGETSNDBUF:
@@ -1273,7 +1258,10 @@ static int tun_chr_ioctl(struct inode *inode, struct file *file,
 		break;
 	};
 
-	tun_put(tun);
+unlock:
+	rtnl_unlock();
+	if (tun)
+		tun_put(tun);
 	return ret;
 }
 
@@ -1361,7 +1349,7 @@ static const struct file_operations tun_fops = {
 	.write = do_sync_write,
 	.aio_write = tun_chr_aio_write,
 	.poll	= tun_chr_poll,
-	.ioctl	= tun_chr_ioctl,
+	.unlocked_ioctl = tun_chr_ioctl,
 	.open	= tun_chr_open,
 	.release = tun_chr_close,
 	.fasync = tun_chr_fasync

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [RFC PATCH v1] tun: Cleanup error handling in tun_set_iff()

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Fri, 7 Aug 2009 10:22:44 +1000

> tun: Extend RTNL lock coverage over whole ioctl
> 
> As it is, parts of the ioctl runs under the RTNL and parts of
> it do not.  The unlocked section is still protected by the BKL,
> but there can be subtle races.  For example, Eric Biederman and
> Paul Moore observed that if two threads tried to create two tun
> devices on the same file descriptor, then unexpected results
> may occur.
> 
> As there isn't anything in the ioctl that is expected to sleep
> indefinitely, we can prevent this from occurring by extending
> the RTNL lock coverage.
> 
> This also allows to get rid of the BKL.
> 
> Finally, I changed tun_get_iff to take a tun device in order to
> avoid calling tun_put which would dead-lockt as it also tries to
> take the RTNL lock.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

This looks good after a quick audit, Eric what say you?

Re: [RFC PATCH v1] tun: Cleanup error handling in tun_set_iff()

[Eric W. Biederman]

David Miller <davem@davemloft.net> writes:

> From: Herbert Xu <herbert@gondor.apana.org.au>
> Date: Fri, 7 Aug 2009 10:22:44 +1000
>
>> tun: Extend RTNL lock coverage over whole ioctl
>> 
>> As it is, parts of the ioctl runs under the RTNL and parts of
>> it do not.  The unlocked section is still protected by the BKL,
>> but there can be subtle races.  For example, Eric Biederman and
>> Paul Moore observed that if two threads tried to create two tun
>> devices on the same file descriptor, then unexpected results
>> may occur.
>> 
>> As there isn't anything in the ioctl that is expected to sleep
>> indefinitely, we can prevent this from occurring by extending
>> the RTNL lock coverage.
>> 
>> This also allows to get rid of the BKL.
>> 
>> Finally, I changed tun_get_iff to take a tun device in order to
>> avoid calling tun_put which would dead-lockt as it also tries to
>> take the RTNL lock.
>> 
>> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
>
> This looks good after a quick audit, Eric what say you?

Looks good to me.

Eric

Re: [RFC PATCH v1] tun: Cleanup error handling in tun_set_iff()

[Paul Moore]

On Thursday 06 August 2009 08:00:21 pm Herbert Xu wrote:
> On Thu, Aug 06, 2009 at 02:20:20PM -0400, Paul Moore wrote:
> > The code currently looks something like this:
> >
> > 	err = -ENOMEM;
> > 	buf = alloc(...);
> > 	if (!buf)
> > 		goto label;
> >
> > This means that in the common case where 'alloc()' completes without
> > error we are doing an extra, unnecessary assignment where we set the
> > value in 'err'. Now, if we change this slightly to match what I proposed
> > in the patch:
> >
> > 	buf = alloc(...);
> > 	if (!buf) {
> > 		err = -ENOMEM;
> > 		goto label;
> > 	}
> >
> > We eliminate that extra assignment in the case where 'alloc()' completes
> > without error, which should result in more efficient code (less
> > instructions in the common case).  Am I wrong?  If that is the case I
> > would appreciate an explanation ...
>
> Your style potentially introduces a second jump which may end
> up being worse compared to the extra work on a modern CPU.

Thanks, I hadn't thought of that possibility.  I suppose the impact of a 
second jump is going to depend quite a bit on the hardware it runs on 
(pipeline depth, branch prediction, etc.) and isn't as easy to quantify as I 
had hoped.  Oh well, I appreciate the explanation anyway :)

-- 
paul moore
linux @ hp

Re: [RFC PATCH v1] tun: Cleanup error handling in tun_set_iff()

[David Miller]

From: ebiederm@xmission.com (Eric W. Biederman)
Date: Thu, 06 Aug 2009 21:22:42 -0700

> David Miller <davem@davemloft.net> writes:
> 
>> From: Herbert Xu <herbert@gondor.apana.org.au>
>> Date: Fri, 7 Aug 2009 10:22:44 +1000
>>
>>> tun: Extend RTNL lock coverage over whole ioctl
>>> 
>>> As it is, parts of the ioctl runs under the RTNL and parts of
>>> it do not.  The unlocked section is still protected by the BKL,
>>> but there can be subtle races.  For example, Eric Biederman and
>>> Paul Moore observed that if two threads tried to create two tun
>>> devices on the same file descriptor, then unexpected results
>>> may occur.
>>> 
>>> As there isn't anything in the ioctl that is expected to sleep
>>> indefinitely, we can prevent this from occurring by extending
>>> the RTNL lock coverage.
>>> 
>>> This also allows to get rid of the BKL.
>>> 
>>> Finally, I changed tun_get_iff to take a tun device in order to
>>> avoid calling tun_put which would dead-lockt as it also tries to
>>> take the RTNL lock.
>>> 
>>> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
>>
>> This looks good after a quick audit, Eric what say you?
> 
> Looks good to me.

Applied, thanks everyone.