tun: Cleanup error handling in tun_set_iff()

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

Signed-off-by Paul Moore <[email protected]>

--

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?

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

Signed-off-by Paul Moore <[email protected]>

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(-)

drivers/net/tun.c +1 -4

index 4a0db7a..b6d06fd 100644
@@ -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;
 

drivers/net/tun.c +0 -0

 		if (*ifr->ifr_name)
 			name = ifr->ifr_name;

drivers/net/tun.c +0 -2

  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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 4 lines · Paul Moore

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

Signed-off-by Paul Moore <[email protected]>

This patch appears slightly wrong. Comments inline.

Quoted 17 lines

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(-)

drivers/net/tun.c +0 -0

index 4a0db7a..b6d06fd 100644
@@ -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;
 

drivers/net/tun.c static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr) · line 980

Quoted 7 lines

drivers/net/tun.c +3 -2

-		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.

drivers/net/tun.c static int tun_set_iff(struct net *net, struct file · line 1013

Quoted 8 lines

drivers/net/tun.c +1 -1

@@ -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.

Quoted 4 lines

drivers/net/tun.c +0 -0

@@ -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;
 

drivers/net/tun.c static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr) · line 1043

Quoted 4 lines

drivers/net/tun.c +4 -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>

Quoted 7 lines · Eric W. Biederman

Quoted 4 lines · Paul Moore

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

Signed-off-by Paul Moore <[email protected]>

This patch appears slightly wrong. Comments inline.

Thanks for taking a look ...

Quoted 17 lines

Quoted 17 lines

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(-)

drivers/net/tun.c +0 -0

index 4a0db7a..b6d06fd 100644
@@ -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;
 

Quoted 9 lines

drivers/net/tun.c static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr) · line 980

Quoted 7 lines

drivers/net/tun.c +3 -2

-		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.

Quoted 17 lines

drivers/net/tun.c static int tun_set_iff(struct net *net, struct file · line 1013

Quoted 8 lines

drivers/net/tun.c +1 -1

@@ -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.

...

Quoted 4 lines

Quoted 4 lines

drivers/net/tun.c +0 -0

@@ -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;
 

Quoted 9 lines

drivers/net/tun.c static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr) · line 1043

Quoted 4 lines

drivers/net/tun.c +4 -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 ... :)

Quoted 7 lines

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?

Quoted 20 lines · Paul Moore

Quoted 18 lines · Eric W. Biederman

Quoted 5 lines · Paul Moore

drivers/net/tun.c +0 -0

index 4a0db7a..b6d06fd 100644

drivers/net/tun.c static int tun_set_iff(struct net *net, struct · line 980

Quoted 10 lines

drivers/net/tun.c +3 -2

@@ -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.

Quoted 3 lines

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?

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 ...

Quoted 18 lines · Paul Moore

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;
	}

Quoted 4 lines

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.

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?

Quoted 6 lines · Paul Moore

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.

Quoted 5 lines · Eric W. Biederman

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().

Quoted 31 lines · Eric W. Biederman

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(..);
            }

Quoted 2 lines

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.

Quoted 34 lines · Herbert Xu

Quoted 32 lines · Eric W. Biederman

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().

Quoted 2 lines

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.

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.

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 <[email protected]>

From: Herbert Xu <[email protected]>
Date: Fri, 7 Aug 2009 10:22:44 +1000

Quoted 15 lines

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.

Quoted 5 lines

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 <[email protected]>

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

Quoted 22 lines · David Miller

From: Herbert Xu <[email protected]>
Date: Fri, 7 Aug 2009 10:22:44 +1000

Quoted 19 lines

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.

Quoted 3 lines

Quoted 1 line

Signed-off-by Herbert Xu <[email protected]>

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

Looks good to me.