[PATCH] Crash in tun

[PATCH] Crash in tun

[Mikulas Patocka]

Hi

I want to report a crash when using the tun driver. The crash can be 
reproduced by starting and stopping miredo in the loop:

while ! dmesg | grep -q BUG; do
	/etc/init.d/miredo start;/etc/init.d/miredo stop
done

The crash happens in iput_final because inode->i_sb->s_op is NULL.

The crash happens in 3.4, 3.4.5, 3.5-rc7. The crash does not happen in 
3.3. When I attempted to bisect, unrelated changes in completely different 
driver regarding memory allocation triggered the crash --- so it is 
likely buggy even in 3.3 and before, it just didn't show up.


What is obviously wrong:
in the tun driver "struct socket" is embedded in "struct tun_struct".

The backtrace goes through:
netdev_run_todo -> tun_free_netdev -> sk_release_kernel -> sock_release.

sock_release calls iput(SOCK_INODE(sock)). SOCK_INODE assumes that struct 
socket is embedded in "struct socket_alloc" (which is not true, it is 
embedded in "struct tun_struct"), gets a pointer to non-existing inode --- 
and there goes the crash.


The crash can be fixed by writing any non-NULL value to tun->socket.file 
to prevent sock_release from calling iput(SOCK_INODE(sock)). Or maybe you 
come up with a better fix.


Note another bug - when you are repeatedly starting and stopping miredo, 
even if it doesn't crash, the value "sockets: used" in 
/proc/*/net/sockstat keeps on decreasing. That's because sock_release 
decrements sockets_in_use, but there was no sock_alloc to increment it.


Mikulas

---

Signed-off-by: Mikulas Patocka <mikulas@artax.karlin.mff.cuni.cz>

---
 drivers/net/tun.c |    2 ++
 net/socket.c      |    4 +++-
 2 files changed, 5 insertions(+), 1 deletion(-)

Index: linux-3.4.5-fast/drivers/net/tun.c
===================================================================
--- linux-3.4.5-fast.orig/drivers/net/tun.c	2012-07-19 02:42:56.000000000 +0200
+++ linux-3.4.5-fast/drivers/net/tun.c	2012-07-19 02:50:13.000000000 +0200
@@ -358,6 +358,8 @@ static void tun_free_netdev(struct net_d
 {
 	struct tun_struct *tun = netdev_priv(dev);
 
+	/* Prevent the code in sock_release from calling iput. */
+	tun->socket.file = (void *)1;
 	sk_release_kernel(tun->socket.sk);
 }
 
Index: linux-3.4.5-fast/net/socket.c
===================================================================
--- linux-3.4.5-fast.orig/net/socket.c	2012-07-19 03:00:30.000000000 +0200
+++ linux-3.4.5-fast/net/socket.c	2012-07-19 03:05:36.000000000 +0200
@@ -522,7 +522,9 @@ void sock_release(struct socket *sock)
 	if (rcu_dereference_protected(sock->wq, 1)->fasync_list)
 		printk(KERN_ERR "sock_release: fasync list not empty!\n");
 
-	percpu_sub(sockets_in_use, 1);
+	/* a hack - sockets_in_use should not be decremented when tun calls this */
+	if (sock->file != (void *)1)
+		percpu_sub(sockets_in_use, 1);
 	if (!sock->file) {
 		iput(SOCK_INODE(sock));
 		return;
 
--- 

BUG: unable to handle kernel NULL pointer dereference at 0000000000000020
IP: [<ffffffff81113246>] iput+0x76/0x230
PGD 43e5e9067 PUD 4468d5067 PMD 0
Oops: 0000 [#1] PREEMPT SMP
CPU 1
Modules linked in: ip6table_filter ip6_tables iptable_filter ip_tables 
ebtable_nat ebtables x_tables kvm_amd kvm tun cpufreq_userspace 
cpufreq_stats cpufreq_powersave cpufreq_ondemand cpufreq_conservative ipv6 
fuse raid0 md_mod lm85 hwmon_vid snd_usb_audio snd_pcm_oss snd_mixer_oss 
snd_pcm snd_timer snd_page_alloc snd_hwdep snd_usbmidi_lib snd_rawmidi snd 
soundcore ide_cd_mod cdrom ohci_hcd sata_svw libata serverworks ide_core 
powernow_k8 ehci_hcd usbcore tg3 usb_common floppy freq_table e100 skge 
mii i2c_piix4 libphy mperf k10temp rtc_cmos processor button hwmon 
microcode unix

Pid: 10826, comm: miredo Not tainted 3.4.5 #85 empty empty/S3992-E
RIP: 0010:[<ffffffff81113246>]  [<ffffffff81113246>] iput+0x76/0x230
RSP: 0018:ffff88043dc73e48  EFLAGS: 00010246
RAX: 0000000000000001 RBX: ffff88023eba3f20 RCX: ffff880447d80000
RDX: 0000000000000001 RSI: ffff88023eba3f98 RDI: ffff88023eba3f98
RBP: ffff88023eba3f98 R08: ffffffff814804d8 R09: 0000000000000000
R10: 0000000000000096 R11: dead000000100100 R12: ffff88023eba3f48
R13: 0000000000000000 R14: ffff88043dc73e88 R15: 00000000ffffad3a
FS:  00007fe71bbb9700(0000) GS:ffff880247c80000(0000) 
knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
CR2: 0000000000000020 CR3: 000000043dcf0000 CR4: 00000000000007f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
Process miredo (pid: 10826, threadinfo ffff88043dc72000, task 
ffff88043dc1b8d0) Stack:
 ffff88043dc73e88 ffff88023efef000 ffff88023eba3800 ffff88023eba3c00
 ffff88024749ecc0 ffffffff81281d93 ffff88023e8fa500 ffffffff81295438
 ffff88043dc73e88 ffff88043dc73e88 ffff88023eba38f8 ffff88023e8fa500
Call Trace:
 [<ffffffff81281d93>] ? sk_release_kernel+0x23/0x40
 [<ffffffff81295438>] ? netdev_run_todo+0x1a8/0x260
 [<ffffffffa02522a3>] ? tun_chr_close+0x93/0xb0 [tun]
 [<ffffffff810fc3ed>] ? fput+0xdd/0x260
 [<ffffffff810f8aff>] ? filp_close+0x5f/0x90
 [<ffffffff810f8bc7>] ? sys_close+0x97/0x100
 [<ffffffff81311f22>] ? system_call_fastpath+0x16/0x1b
Code: 6c 24 10 4c 8b 64 24 18 4c 8b 6c 24 20 48 83 c4 28 c3 0f 1f 00 f6 83 
90 00 00 00 08 4c 8b 63 28 4d 8b 6c 24 30 0f 85 7d 01 00 00 <49> 8b 45 20 
48 85 c0 0f 84 9d 00 00 00 48 89 df ff d0 85 c0 0f
RIP  [<ffffffff81113246>] iput+0x76/0x230
 RSP <ffff88043dc73e48>
CR2: 0000000000000020
---[ end trace 6bd5160ffd3ba7a2 ]---
note: miredo[10826] exited with preempt_count 1

Re: [PATCH] Crash in tun

[Eric Dumazet]

On Thu, 2012-07-19 at 03:12 +0200, Mikulas Patocka wrote:
> Hi
> 
> I want to report a crash when using the tun driver. The crash can be 
> reproduced by starting and stopping miredo in the loop:
> 
> while ! dmesg | grep -q BUG; do
> 	/etc/init.d/miredo start;/etc/init.d/miredo stop
> done
> 
> The crash happens in iput_final because inode->i_sb->s_op is NULL.
> 
> The crash happens in 3.4, 3.4.5, 3.5-rc7. The crash does not happen in 
> 3.3. When I attempted to bisect, unrelated changes in completely different 
> driver regarding memory allocation triggered the crash --- so it is 
> likely buggy even in 3.3 and before, it just didn't show up.
> 
> 
> What is obviously wrong:
> in the tun driver "struct socket" is embedded in "struct tun_struct".
> 
> The backtrace goes through:
> netdev_run_todo -> tun_free_netdev -> sk_release_kernel -> sock_release.
> 
> sock_release calls iput(SOCK_INODE(sock)). SOCK_INODE assumes that struct 
> socket is embedded in "struct socket_alloc" (which is not true, it is 
> embedded in "struct tun_struct"), gets a pointer to non-existing inode --- 
> and there goes the crash.
> 
> 
> The crash can be fixed by writing any non-NULL value to tun->socket.file 
> to prevent sock_release from calling iput(SOCK_INODE(sock)). Or maybe you 
> come up with a better fix.
> 
> 
> Note another bug - when you are repeatedly starting and stopping miredo, 
> even if it doesn't crash, the value "sockets: used" in 
> /proc/*/net/sockstat keeps on decreasing. That's because sock_release 
> decrements sockets_in_use, but there was no sock_alloc to increment it.
> 
> 
> Mikulas
> 
> ---
> 
> Signed-off-by: Mikulas Patocka <mikulas@artax.karlin.mff.cuni.cz>
> 
> ---
>  drivers/net/tun.c |    2 ++
>  net/socket.c      |    4 +++-
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> Index: linux-3.4.5-fast/drivers/net/tun.c
> ===================================================================
> --- linux-3.4.5-fast.orig/drivers/net/tun.c	2012-07-19 02:42:56.000000000 +0200
> +++ linux-3.4.5-fast/drivers/net/tun.c	2012-07-19 02:50:13.000000000 +0200
> @@ -358,6 +358,8 @@ static void tun_free_netdev(struct net_d
>  {
>  	struct tun_struct *tun = netdev_priv(dev);
>  
> +	/* Prevent the code in sock_release from calling iput. */
> +	tun->socket.file = (void *)1;
>  	sk_release_kernel(tun->socket.sk);
>  }
>  
> Index: linux-3.4.5-fast/net/socket.c
> ===================================================================
> --- linux-3.4.5-fast.orig/net/socket.c	2012-07-19 03:00:30.000000000 +0200
> +++ linux-3.4.5-fast/net/socket.c	2012-07-19 03:05:36.000000000 +0200
> @@ -522,7 +522,9 @@ void sock_release(struct socket *sock)
>  	if (rcu_dereference_protected(sock->wq, 1)->fasync_list)
>  		printk(KERN_ERR "sock_release: fasync list not empty!\n");
>  
> -	percpu_sub(sockets_in_use, 1);
> +	/* a hack - sockets_in_use should not be decremented when tun calls this */
> +	if (sock->file != (void *)1)
> +		percpu_sub(sockets_in_use, 1);
>  	if (!sock->file) {
>  		iput(SOCK_INODE(sock));
>  		return;
>  
> --- 
> 
> BUG: unable to handle kernel NULL pointer dereference at 0000000000000020
> IP: [<ffffffff81113246>] iput+0x76/0x230
> PGD 43e5e9067 PUD 4468d5067 PMD 0
> Oops: 0000 [#1] PREEMPT SMP
> CPU 1
> Modules linked in: ip6table_filter ip6_tables iptable_filter ip_tables 
> ebtable_nat ebtables x_tables kvm_amd kvm tun cpufreq_userspace 
> cpufreq_stats cpufreq_powersave cpufreq_ondemand cpufreq_conservative ipv6 
> fuse raid0 md_mod lm85 hwmon_vid snd_usb_audio snd_pcm_oss snd_mixer_oss 
> snd_pcm snd_timer snd_page_alloc snd_hwdep snd_usbmidi_lib snd_rawmidi snd 
> soundcore ide_cd_mod cdrom ohci_hcd sata_svw libata serverworks ide_core 
> powernow_k8 ehci_hcd usbcore tg3 usb_common floppy freq_table e100 skge 
> mii i2c_piix4 libphy mperf k10temp rtc_cmos processor button hwmon 
> microcode unix
> 
> Pid: 10826, comm: miredo Not tainted 3.4.5 #85 empty empty/S3992-E
> RIP: 0010:[<ffffffff81113246>]  [<ffffffff81113246>] iput+0x76/0x230
> RSP: 0018:ffff88043dc73e48  EFLAGS: 00010246
> RAX: 0000000000000001 RBX: ffff88023eba3f20 RCX: ffff880447d80000
> RDX: 0000000000000001 RSI: ffff88023eba3f98 RDI: ffff88023eba3f98
> RBP: ffff88023eba3f98 R08: ffffffff814804d8 R09: 0000000000000000
> R10: 0000000000000096 R11: dead000000100100 R12: ffff88023eba3f48
> R13: 0000000000000000 R14: ffff88043dc73e88 R15: 00000000ffffad3a
> FS:  00007fe71bbb9700(0000) GS:ffff880247c80000(0000) 
> knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
> CR2: 0000000000000020 CR3: 000000043dcf0000 CR4: 00000000000007f0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
> Process miredo (pid: 10826, threadinfo ffff88043dc72000, task 
> ffff88043dc1b8d0) Stack:
>  ffff88043dc73e88 ffff88023efef000 ffff88023eba3800 ffff88023eba3c00
>  ffff88024749ecc0 ffffffff81281d93 ffff88023e8fa500 ffffffff81295438
>  ffff88043dc73e88 ffff88043dc73e88 ffff88023eba38f8 ffff88023e8fa500
> Call Trace:
>  [<ffffffff81281d93>] ? sk_release_kernel+0x23/0x40
>  [<ffffffff81295438>] ? netdev_run_todo+0x1a8/0x260
>  [<ffffffffa02522a3>] ? tun_chr_close+0x93/0xb0 [tun]
>  [<ffffffff810fc3ed>] ? fput+0xdd/0x260
>  [<ffffffff810f8aff>] ? filp_close+0x5f/0x90
>  [<ffffffff810f8bc7>] ? sys_close+0x97/0x100
>  [<ffffffff81311f22>] ? system_call_fastpath+0x16/0x1b
> Code: 6c 24 10 4c 8b 64 24 18 4c 8b 6c 24 20 48 83 c4 28 c3 0f 1f 00 f6 83 
> 90 00 00 00 08 4c 8b 63 28 4d 8b 6c 24 30 0f 85 7d 01 00 00 <49> 8b 45 20 
> 48 85 c0 0f 84 9d 00 00 00 48 89 df ff d0 85 c0 0f
> RIP  [<ffffffff81113246>] iput+0x76/0x230
>  RSP <ffff88043dc73e48>
> CR2: 0000000000000020
> ---[ end trace 6bd5160ffd3ba7a2 ]---
> note: miredo[10826] exited with preempt_count 1


Hi Mikulas

A fix for this problem is : http://patchwork.ozlabs.org/patch/170440/

Re: [PATCH] Crash in tun

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Thu, 19 Jul 2012 08:09:35 +0200

> A fix for this problem is : http://patchwork.ozlabs.org/patch/170440/

But it was submitted as an RFC so much be resubmitted formally
as a non-RFC patch.

Re: [PATCH] Crash in tun

[Mikulas Patocka]

On Thu, 19 Jul 2012, Eric Dumazet wrote:

> Hi Mikulas
> 
> A fix for this problem is : http://patchwork.ozlabs.org/patch/170440/

If you call tun_free_netdev beacuse of a jump to an error label 
err_free_sk, your patch still calls it with NULL file, causing a memory 
corruption and a possible crash.

Your patch doesn't fix sockets_in_use underflow.

Maybe we can commit this patch --- it introduces a new flag 
SOCK_EXTERNALLY_ALLOCATED to work around both problems. (it looks quite 
nicer than my previous patch with file = (void *)1).

Mikulas

---

tun: fix a crash bug and a memory leak

This patch fixes a crash
tun_chr_close -> netdev_run_todo -> tun_free_netdev -> sk_release_kernel ->
sock_release -> iput(SOCK_INODE(sock))
introduced by commit 1ab5ecb90cb6a3df1476e052f76a6e8f6511cb3d

The problem is that this socket is embedded in struct tun_struct, it has
no inode, iput is called on invalid inode, which modifies invalid memory
and optionally causes a crash.

sock_release also decrements sockets_in_use, this causes a bug that
"sockets: used" field in /proc/*/net/sockstat keeps on decreasing when
creating and closing tun devices.

This patch introduces a flag SOCK_EXTERNALLY_ALLOCATED that instructs
sock_release to not free the inode and not decrement sockets_in_use,
fixing both memory corruption and sockets_in_use underflow.

It should be backported to 3.3 an 3.4 stabke.

Signed-off-by: Mikulas Patocka <mikulas@artax.karlin.mff.cuni.cz>
Cc: stable@kernel.org

---
 drivers/net/tun.c   |    3 +++
 include/linux/net.h |    1 +
 net/socket.c        |    3 +++
 3 files changed, 7 insertions(+)

Index: linux-3.4.5-fast/drivers/net/tun.c
===================================================================
--- linux-3.4.5-fast.orig/drivers/net/tun.c	2012-07-19 17:55:16.000000000 +0200
+++ linux-3.4.5-fast/drivers/net/tun.c	2012-07-19 17:58:30.000000000 +0200
@@ -358,6 +358,8 @@ static void tun_free_netdev(struct net_d
 {
 	struct tun_struct *tun = netdev_priv(dev);
 
+	BUG_ON(!test_bit(SOCK_EXTERNALLY_ALLOCATED, &tun->socket.flags));
+
 	sk_release_kernel(tun->socket.sk);
 }
 
@@ -1115,6 +1117,7 @@ static int tun_set_iff(struct net *net,
 		tun->flags = flags;
 		tun->txflt.count = 0;
 		tun->vnet_hdr_sz = sizeof(struct virtio_net_hdr);
+		set_bit(SOCK_EXTERNALLY_ALLOCATED, &tun->socket.flags);
 
 		err = -ENOMEM;
 		sk = sk_alloc(&init_net, AF_UNSPEC, GFP_KERNEL, &tun_proto);
Index: linux-3.4.5-fast/include/linux/net.h
===================================================================
--- linux-3.4.5-fast.orig/include/linux/net.h	2012-07-19 17:54:31.000000000 +0200
+++ linux-3.4.5-fast/include/linux/net.h	2012-07-19 17:55:03.000000000 +0200
@@ -72,6 +72,7 @@ struct net;
 #define SOCK_NOSPACE		2
 #define SOCK_PASSCRED		3
 #define SOCK_PASSSEC		4
+#define SOCK_EXTERNALLY_ALLOCATED 5
 
 #ifndef ARCH_HAS_SOCKET_TYPES
 /**
Index: linux-3.4.5-fast/net/socket.c
===================================================================
--- linux-3.4.5-fast.orig/net/socket.c	2012-07-19 17:56:55.000000000 +0200
+++ linux-3.4.5-fast/net/socket.c	2012-07-19 17:57:50.000000000 +0200
@@ -522,6 +522,9 @@ void sock_release(struct socket *sock)
 	if (rcu_dereference_protected(sock->wq, 1)->fasync_list)
 		printk(KERN_ERR "sock_release: fasync list not empty!\n");
 
+	if (test_bit(SOCK_EXTERNALLY_ALLOCATED, &sock->flags))
+		return;
+
 	percpu_sub(sockets_in_use, 1);
 	if (!sock->file) {
 		iput(SOCK_INODE(sock));

Re: [PATCH] Crash in tun

[Max Krasnyansky]

On 07/19/2012 09:13 AM, Mikulas Patocka wrote:
> 
> 
> On Thu, 19 Jul 2012, Eric Dumazet wrote:
> 
>> Hi Mikulas
>>
>> A fix for this problem is : http://patchwork.ozlabs.org/patch/170440/
> 
> If you call tun_free_netdev beacuse of a jump to an error label 
> err_free_sk, your patch still calls it with NULL file, causing a memory 
> corruption and a possible crash.
> 
> Your patch doesn't fix sockets_in_use underflow.
> 
> Maybe we can commit this patch --- it introduces a new flag 
> SOCK_EXTERNALLY_ALLOCATED to work around both problems. (it looks quite 
> nicer than my previous patch with file = (void *)1).


I definitely like this second version better. Less hacky an all.

btw I don't remember now who added the socket business to tun_struct and why.
It seems to be messy in general. Originally version, back when I was still paying attention to it,
didn't have sockets at all. Only char and net devices. It was much cleaner.

Max






> ---
> 
> tun: fix a crash bug and a memory leak
> 
> This patch fixes a crash
> tun_chr_close -> netdev_run_todo -> tun_free_netdev -> sk_release_kernel ->
> sock_release -> iput(SOCK_INODE(sock))
> introduced by commit 1ab5ecb90cb6a3df1476e052f76a6e8f6511cb3d
> 
> The problem is that this socket is embedded in struct tun_struct, it has
> no inode, iput is called on invalid inode, which modifies invalid memory
> and optionally causes a crash.
> 
> sock_release also decrements sockets_in_use, this causes a bug that
> "sockets: used" field in /proc/*/net/sockstat keeps on decreasing when
> creating and closing tun devices.
> 
> This patch introduces a flag SOCK_EXTERNALLY_ALLOCATED that instructs
> sock_release to not free the inode and not decrement sockets_in_use,
> fixing both memory corruption and sockets_in_use underflow.
> 
> It should be backported to 3.3 an 3.4 stabke.
> 
> Signed-off-by: Mikulas Patocka <mikulas@artax.karlin.mff.cuni.cz>
> Cc: stable@kernel.org
> 
> ---
>  drivers/net/tun.c   |    3 +++
>  include/linux/net.h |    1 +
>  net/socket.c        |    3 +++
>  3 files changed, 7 insertions(+)
> 
> Index: linux-3.4.5-fast/drivers/net/tun.c
> ===================================================================
> --- linux-3.4.5-fast.orig/drivers/net/tun.c	2012-07-19 17:55:16.000000000 +0200
> +++ linux-3.4.5-fast/drivers/net/tun.c	2012-07-19 17:58:30.000000000 +0200
> @@ -358,6 +358,8 @@ static void tun_free_netdev(struct net_d
>  {
>  	struct tun_struct *tun = netdev_priv(dev);
>  
> +	BUG_ON(!test_bit(SOCK_EXTERNALLY_ALLOCATED, &tun->socket.flags));
> +
>  	sk_release_kernel(tun->socket.sk);
>  }
>  
> @@ -1115,6 +1117,7 @@ static int tun_set_iff(struct net *net,
>  		tun->flags = flags;
>  		tun->txflt.count = 0;
>  		tun->vnet_hdr_sz = sizeof(struct virtio_net_hdr);
> +		set_bit(SOCK_EXTERNALLY_ALLOCATED, &tun->socket.flags);
>  
>  		err = -ENOMEM;
>  		sk = sk_alloc(&init_net, AF_UNSPEC, GFP_KERNEL, &tun_proto);
> Index: linux-3.4.5-fast/include/linux/net.h
> ===================================================================
> --- linux-3.4.5-fast.orig/include/linux/net.h	2012-07-19 17:54:31.000000000 +0200
> +++ linux-3.4.5-fast/include/linux/net.h	2012-07-19 17:55:03.000000000 +0200
> @@ -72,6 +72,7 @@ struct net;
>  #define SOCK_NOSPACE		2
>  #define SOCK_PASSCRED		3
>  #define SOCK_PASSSEC		4
> +#define SOCK_EXTERNALLY_ALLOCATED 5
>  
>  #ifndef ARCH_HAS_SOCKET_TYPES
>  /**
> Index: linux-3.4.5-fast/net/socket.c
> ===================================================================
> --- linux-3.4.5-fast.orig/net/socket.c	2012-07-19 17:56:55.000000000 +0200
> +++ linux-3.4.5-fast/net/socket.c	2012-07-19 17:57:50.000000000 +0200
> @@ -522,6 +522,9 @@ void sock_release(struct socket *sock)
>  	if (rcu_dereference_protected(sock->wq, 1)->fasync_list)
>  		printk(KERN_ERR "sock_release: fasync list not empty!\n");
>  
> +	if (test_bit(SOCK_EXTERNALLY_ALLOCATED, &sock->flags))
> +		return;
> +
>  	percpu_sub(sockets_in_use, 1);
>  	if (!sock->file) {
>  		iput(SOCK_INODE(sock));
> 

Re: [PATCH] Crash in tun

[David Miller]

From: Max Krasnyansky <maxk@qualcomm.com>
Date: Thu, 19 Jul 2012 10:44:01 -0700

> btw I don't remember now who added the socket business to tun_struct and why.

Is GIT really so broken on your computer that you can't find the
answer to this question in like 5 seconds as I just did?

commit 33dccbb050bbe35b88ca8cf1228dcf3e4d4b3554
Author: Herbert Xu <herbert@gondor.apana.org.au>
Date:   Thu Feb 5 21:25:32 2009 -0800

    tun: Limit amount of queued packets per device
    
    Unlike a normal socket path, the tuntap device send path does
    not have any accounting.  This means that the user-space sender
    may be able to pin down arbitrary amounts of kernel memory by
    continuing to send data to an end-point that is congested.
    
    Even when this isn't an issue because of limited queueing at
    most end points, this can also be a problem because its only
    response to congestion is packet loss.  That is, when those
    local queues at the end-point fills up, the tuntap device will
    start wasting system time because it will continue to send
    data there which simply gets dropped straight away.
    
    Of course one could argue that everybody should do congestion
    control end-to-end, unfortunately there are people in this world
    still hooked on UDP, and they don't appear to be going away
    anywhere fast.  In fact, we've always helped them by performing
    accounting in our UDP code, the sole purpose of which is to
    provide congestion feedback other than through packet loss.
    
    This patch attempts to apply the same bandaid to the tuntap device.
    It creates a pseudo-socket object which is used to account our
    packets just as a normal socket does for UDP.  Of course things
    are a little complex because we're actually reinjecting traffic
    back into the stack rather than out of the stack.
    
    The stack complexities however should have been resolved by preceding
    patches.  So this one can simply start using skb_set_owner_w.
    
    For now the accounting is essentially disabled by default for
    backwards compatibility.  In particular, we set the cap to INT_MAX.
    This is so that existing applications don't get confused by the
    sudden arrival EAGAIN errors.
    
    In future we may wish (or be forced to) do this by default.
    
    Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
    Signed-off-by: David S. Miller <davem@davemloft.net>

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 15d6763..0476549 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -64,6 +64,7 @@
 #include <net/net_namespace.h>
 #include <net/netns/generic.h>
 #include <net/rtnetlink.h>
+#include <net/sock.h>
 
 #include <asm/system.h>
 #include <asm/uaccess.h>
@@ -95,6 +96,8 @@ struct tun_file {
 	wait_queue_head_t	read_wait;
 };
 
+struct tun_sock;
+
 struct tun_struct {
 	struct tun_file		*tfile;
 	unsigned int 		flags;
@@ -107,12 +110,24 @@ struct tun_struct {
 	struct fasync_struct	*fasync;
 
 	struct tap_filter       txflt;
+	struct sock		*sk;
+	struct socket		socket;
 
 #ifdef TUN_DEBUG
 	int debug;
 #endif
 };
 
+struct tun_sock {
+	struct sock		sk;
+	struct tun_struct	*tun;
+};
+
+static inline struct tun_sock *tun_sk(struct sock *sk)
+{
+	return container_of(sk, struct tun_sock, sk);
+}
+
 static int tun_attach(struct tun_struct *tun, struct file *file)
 {
 	struct tun_file *tfile = file->private_data;
@@ -461,7 +476,8 @@ static unsigned int tun_chr_poll(struct file *file, poll_table * wait)
 {
 	struct tun_file *tfile = file->private_data;
 	struct tun_struct *tun = __tun_get(tfile);
-	unsigned int mask = POLLOUT | POLLWRNORM;
+	struct sock *sk = tun->sk;
+	unsigned int mask = 0;
 
 	if (!tun)
 		return POLLERR;
@@ -473,6 +489,11 @@ static unsigned int tun_chr_poll(struct file *file, poll_table * wait)
 	if (!skb_queue_empty(&tun->readq))
 		mask |= POLLIN | POLLRDNORM;
 
+	if (sock_writeable(sk) ||
+	    (!test_and_set_bit(SOCK_ASYNC_NOSPACE, &sk->sk_socket->flags) &&
+	     sock_writeable(sk)))
+		mask |= POLLOUT | POLLWRNORM;
+
 	if (tun->dev->reg_state != NETREG_REGISTERED)
 		mask = POLLERR;
 
@@ -482,66 +503,35 @@ static unsigned int tun_chr_poll(struct file *file, poll_table * wait)
 
 /* prepad is the amount to reserve at front.  len is length after that.
  * linear is a hint as to how much to copy (usually headers). */
-static struct sk_buff *tun_alloc_skb(size_t prepad, size_t len, size_t linear,
-				     gfp_t gfp)
+static inline struct sk_buff *tun_alloc_skb(struct tun_struct *tun,
+					    size_t prepad, size_t len,
+					    size_t linear, int noblock)
 {
+	struct sock *sk = tun->sk;
 	struct sk_buff *skb;
-	unsigned int i;
-
-	skb = alloc_skb(prepad + len, gfp|__GFP_NOWARN);
-	if (skb) {
-		skb_reserve(skb, prepad);
-		skb_put(skb, len);
-		return skb;
-	}
+	int err;
 
 	/* Under a page?  Don't bother with paged skb. */
 	if (prepad + len < PAGE_SIZE)
-		return NULL;
+		linear = len;
 
-	/* Start with a normal skb, and add pages. */
-	skb = alloc_skb(prepad + linear, gfp);
+	skb = sock_alloc_send_pskb(sk, prepad + linear, len - linear, noblock,
+				   &err);
 	if (!skb)
-		return NULL;
+		return ERR_PTR(err);
 
 	skb_reserve(skb, prepad);
 	skb_put(skb, linear);
-
-	len -= linear;
-
-	for (i = 0; i < MAX_SKB_FRAGS; i++) {
-		skb_frag_t *f = &skb_shinfo(skb)->frags[i];
-
-		f->page = alloc_page(gfp|__GFP_ZERO);
-		if (!f->page)
-			break;
-
-		f->page_offset = 0;
-		f->size = PAGE_SIZE;
-
-		skb->data_len += PAGE_SIZE;
-		skb->len += PAGE_SIZE;
-		skb->truesize += PAGE_SIZE;
-		skb_shinfo(skb)->nr_frags++;
-
-		if (len < PAGE_SIZE) {
-			len = 0;
-			break;
-		}
-		len -= PAGE_SIZE;
-	}
-
-	/* Too large, or alloc fail? */
-	if (unlikely(len)) {
-		kfree_skb(skb);
-		skb = NULL;
-	}
+	skb->data_len = len - linear;
+	skb->len += len - linear;
 
 	return skb;
 }
 
 /* Get packet from user space buffer */
-static __inline__ ssize_t tun_get_user(struct tun_struct *tun, struct iovec *iv, size_t count)
+static __inline__ ssize_t tun_get_user(struct tun_struct *tun,
+				       struct iovec *iv, size_t count,
+				       int noblock)
 {
 	struct tun_pi pi = { 0, cpu_to_be16(ETH_P_IP) };
 	struct sk_buff *skb;
@@ -573,9 +563,11 @@ static __inline__ ssize_t tun_get_user(struct tun_struct *tun, struct iovec *iv,
 			return -EINVAL;
 	}
 
-	if (!(skb = tun_alloc_skb(align, len, gso.hdr_len, GFP_KERNEL))) {
-		tun->dev->stats.rx_dropped++;
-		return -ENOMEM;
+	skb = tun_alloc_skb(tun, align, len, gso.hdr_len, noblock);
+	if (IS_ERR(skb)) {
+		if (PTR_ERR(skb) != -EAGAIN)
+			tun->dev->stats.rx_dropped++;
+		return PTR_ERR(skb);
 	}
 
 	if (skb_copy_datagram_from_iovec(skb, 0, iv, len)) {
@@ -661,7 +653,8 @@ static __inline__ ssize_t tun_get_user(struct tun_struct *tun, struct iovec *iv,
 static ssize_t tun_chr_aio_write(struct kiocb *iocb, const struct iovec *iv,
 			      unsigned long count, loff_t pos)
 {
-	struct tun_struct *tun = tun_get(iocb->ki_filp);
+	struct file *file = iocb->ki_filp;
+	struct tun_struct *tun = file->private_data;
 	ssize_t result;
 
 	if (!tun)
@@ -669,7 +662,8 @@ static ssize_t tun_chr_aio_write(struct kiocb *iocb, const struct iovec *iv,
 
 	DBG(KERN_INFO "%s: tun_chr_write %ld\n", tun->dev->name, count);
 
-	result = tun_get_user(tun, (struct iovec *) iv, iov_length(iv, count));
+	result = tun_get_user(tun, (struct iovec *)iv, iov_length(iv, count),
+			      file->f_flags & O_NONBLOCK);
 
 	tun_put(tun);
 	return result;
@@ -828,11 +822,40 @@ static struct rtnl_link_ops tun_link_ops __read_mostly = {
 	.validate	= tun_validate,
 };
 
+static void tun_sock_write_space(struct sock *sk)
+{
+	struct tun_struct *tun;
+
+	if (!sock_writeable(sk))
+		return;
+
+	if (sk->sk_sleep && waitqueue_active(sk->sk_sleep))
+		wake_up_interruptible_sync(sk->sk_sleep);
+
+	if (!test_and_clear_bit(SOCK_ASYNC_NOSPACE, &sk->sk_socket->flags))
+		return;
+
+	tun = container_of(sk, struct tun_sock, sk)->tun;
+	kill_fasync(&tun->fasync, SIGIO, POLL_OUT);
+}
+
+static void tun_sock_destruct(struct sock *sk)
+{
+	dev_put(container_of(sk, struct tun_sock, sk)->tun->dev);
+}
+
+static struct proto tun_proto = {
+	.name		= "tun",
+	.owner		= THIS_MODULE,
+	.obj_size	= sizeof(struct tun_sock),
+};
 
 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);
@@ -885,14 +908,31 @@ 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)
+			goto err_free_dev;
+
+		/* This ref count is for tun->sk. */
+		dev_hold(dev);
+		sock_init_data(&tun->socket, sk);
+		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;
+
 		tun_net_init(dev);
 
 		if (strchr(dev->name, '%')) {
 			err = dev_alloc_name(dev, dev->name);
 			if (err < 0)
-				goto err_free_dev;
+				goto err_free_sk;
 		}
 
+		err = -EINVAL;
 		err = register_netdevice(tun->dev);
 		if (err < 0)
 			goto err_free_dev;
@@ -928,6 +968,8 @@ static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr)
 	strcpy(ifr->ifr_name, tun->dev->name);
 	return 0;
 
+ err_free_sk:
+	sock_put(sk);
  err_free_dev:
 	free_netdev(dev);
  failed:
@@ -1012,6 +1054,7 @@ static int tun_chr_ioctl(struct inode *inode, struct file *file,
 	struct tun_struct *tun;
 	void __user* argp = (void __user*)arg;
 	struct ifreq ifr;
+	int sndbuf;
 	int ret;
 
 	if (cmd == TUNSETIFF || _IOC_TYPE(cmd) == 0x89)
@@ -1151,6 +1194,22 @@ static int tun_chr_ioctl(struct inode *inode, struct file *file,
 		ret = dev_set_mac_address(tun->dev, &ifr.ifr_hwaddr);
 		rtnl_unlock();
 		break;
+
+	case TUNGETSNDBUF:
+		sndbuf = tun->sk->sk_sndbuf;
+		if (copy_to_user(argp, &sndbuf, sizeof(sndbuf)))
+			ret = -EFAULT;
+		break;
+
+	case TUNSETSNDBUF:
+		if (copy_from_user(&sndbuf, argp, sizeof(sndbuf))) {
+			ret = -EFAULT;
+			break;
+		}
+
+		tun->sk->sk_sndbuf = sndbuf;
+		break;
+
 	default:
 		ret = -EINVAL;
 		break;
@@ -1218,8 +1277,10 @@ static int tun_chr_close(struct inode *inode, struct file *file)
 		__tun_detach(tun);
 
 		/* If desireable, unregister the netdevice. */
-		if (!(tun->flags & TUN_PERSIST))
+		if (!(tun->flags & TUN_PERSIST)) {
+			sock_put(tun->sk);
 			unregister_netdevice(tun->dev);
+		}
 
 		rtnl_unlock();
 	}
diff --git a/fs/compat_ioctl.c b/fs/compat_ioctl.c
index c8f8d59..c03c10d 100644
--- a/fs/compat_ioctl.c
+++ b/fs/compat_ioctl.c
@@ -1988,6 +1988,8 @@ COMPATIBLE_IOCTL(TUNSETGROUP)
 COMPATIBLE_IOCTL(TUNGETFEATURES)
 COMPATIBLE_IOCTL(TUNSETOFFLOAD)
 COMPATIBLE_IOCTL(TUNSETTXFILTER)
+COMPATIBLE_IOCTL(TUNGETSNDBUF)
+COMPATIBLE_IOCTL(TUNSETSNDBUF)
 /* Big V */
 COMPATIBLE_IOCTL(VT_SETMODE)
 COMPATIBLE_IOCTL(VT_GETMODE)
diff --git a/include/linux/if_tun.h b/include/linux/if_tun.h
index 8529f57..049d6c9 100644
--- a/include/linux/if_tun.h
+++ b/include/linux/if_tun.h
@@ -46,6 +46,8 @@
 #define TUNSETOFFLOAD  _IOW('T', 208, unsigned int)
 #define TUNSETTXFILTER _IOW('T', 209, unsigned int)
 #define TUNGETIFF      _IOR('T', 210, unsigned int)
+#define TUNGETSNDBUF   _IOR('T', 211, int)
+#define TUNSETSNDBUF   _IOW('T', 212, int)
 
 /* TUNSETIFF ifr flags */
 #define IFF_TUN		0x0001

Re: [PATCH] Crash in tun

[Max Krasnyansky]

On 07/19/2012 10:47 AM, David Miller wrote:
> From: Max Krasnyansky <maxk@qualcomm.com>
> Date: Thu, 19 Jul 2012 10:44:01 -0700
> 
>> btw I don't remember now who added the socket business to tun_struct and why.
> 
> Is GIT really so broken on your computer that you can't find the
> answer to this question in like 5 seconds as I just did?

No. I'm just too lazy these days. Too much surfing I guess :).

> commit 33dccbb050bbe35b88ca8cf1228dcf3e4d4b3554
> Author: Herbert Xu <herbert@gondor.apana.org.au>
> Date:   Thu Feb 5 21:25:32 2009 -0800
> 
>     tun: Limit amount of queued packets per device
> <snip>     
>     This patch attempts to apply the same bandaid to the tuntap device.
>     It creates a pseudo-socket object which is used to account our
>     packets just as a normal socket does for UDP.  Of course things
>     are a little complex because we're actually reinjecting traffic
>     back into the stack rather than out of the stack.

Thanks for the info. Overall it definitely makes sense. Still feels a bit of an overkill.
i.e. That we need to allocated a socket just for accounting but I guess all the involved
skb primitives are heavily based on that. If there are other use cases like this maybe
it makes sense to factor accounting stuff out of the socket struct?

Max

Re: [PATCH] Crash in tun

[David Miller]

From: Mikulas Patocka <mikulas@artax.karlin.mff.cuni.cz>
Date: Thu, 19 Jul 2012 18:13:36 +0200 (CEST)

> tun: fix a crash bug and a memory leak
> 
> This patch fixes a crash
> tun_chr_close -> netdev_run_todo -> tun_free_netdev -> sk_release_kernel ->
> sock_release -> iput(SOCK_INODE(sock))
> introduced by commit 1ab5ecb90cb6a3df1476e052f76a6e8f6511cb3d
> 
> The problem is that this socket is embedded in struct tun_struct, it has
> no inode, iput is called on invalid inode, which modifies invalid memory
> and optionally causes a crash.
> 
> sock_release also decrements sockets_in_use, this causes a bug that
> "sockets: used" field in /proc/*/net/sockstat keeps on decreasing when
> creating and closing tun devices.
> 
> This patch introduces a flag SOCK_EXTERNALLY_ALLOCATED that instructs
> sock_release to not free the inode and not decrement sockets_in_use,
> fixing both memory corruption and sockets_in_use underflow.
> 
> It should be backported to 3.3 an 3.4 stabke.
> 
> Signed-off-by: Mikulas Patocka <mikulas@artax.karlin.mff.cuni.cz>
> Cc: stable@kernel.org

Applied.

Re: [PATCH] Crash in tun

[Al Viro]

	BTW, speaking of struct file treatment related to sockets -
there's this piece of code in iscsi:
        /*
         * The SCTP stack needs struct socket->file.
         */
        if ((np->np_network_transport == ISCSI_SCTP_TCP) ||
            (np->np_network_transport == ISCSI_SCTP_UDP)) {
                if (!new_sock->file) {
                        new_sock->file = kzalloc(
                                        sizeof(struct file), GFP_KERNEL);

For one thing, as far as I can see it'not true - sctp does *not* depend on
socket->file being non-NULL; it does, in one place, check socket->file->f_flags
for O_NONBLOCK, but there it treats NULL socket->file as "flag not set".
Which is the case here anyway - the fake struct file created in
__iscsi_target_login_thread() (and in iscsi_target_setup_login_socket(), with
the same excuse) do *not* get that flag set.

Moreover, it's a bloody serious violation of a bunch of asserts in VFS;
all struct file instances should come from filp_cachep, via get_empty_filp()
(or alloc_file(), which is a wrapper for it).  FWIW, I'm very tempted to
do this and be done with the entire mess:

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
index d57d10c..d7dcd67 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -429,19 +429,8 @@ int iscsit_reset_np_thread(
 
 int iscsit_del_np_comm(struct iscsi_np *np)
 {
-	if (!np->np_socket)
-		return 0;
-
-	/*
-	 * Some network transports allocate their own struct sock->file,
-	 * see  if we need to free any additional allocated resources.
-	 */
-	if (np->np_flags & NPF_SCTP_STRUCT_FILE) {
-		kfree(np->np_socket->file);
-		np->np_socket->file = NULL;
-	}
-
-	sock_release(np->np_socket);
+	if (np->np_socket)
+		sock_release(np->np_socket);
 	return 0;
 }
 
@@ -4077,13 +4066,8 @@ int iscsit_close_connection(
 	kfree(conn->conn_ops);
 	conn->conn_ops = NULL;
 
-	if (conn->sock) {
-		if (conn->conn_flags & CONNFLAG_SCTP_STRUCT_FILE) {
-			kfree(conn->sock->file);
-			conn->sock->file = NULL;
-		}
+	if (conn->sock)
 		sock_release(conn->sock);
-	}
 	conn->thread_set = NULL;
 
 	pr_debug("Moving to TARG_CONN_STATE_FREE.\n");
diff --git a/drivers/target/iscsi/iscsi_target_core.h b/drivers/target/iscsi/iscsi_target_core.h
index 1c70144..1dd5716 100644
--- a/drivers/target/iscsi/iscsi_target_core.h
+++ b/drivers/target/iscsi/iscsi_target_core.h
@@ -224,7 +224,6 @@ enum iscsi_timer_flags_table {
 /* Used for struct iscsi_np->np_flags */
 enum np_flags_table {
 	NPF_IP_NETWORK		= 0x00,
-	NPF_SCTP_STRUCT_FILE	= 0x01 /* Bugfix */
 };
 
 /* Used for struct iscsi_np->np_thread_state */
@@ -503,7 +502,6 @@ struct iscsi_conn {
 	u16			local_port;
 	int			net_size;
 	u32			auth_id;
-#define CONNFLAG_SCTP_STRUCT_FILE			0x01
 	u32			conn_flags;
 	/* Used for iscsi_tx_login_rsp() */
 	u32			login_itt;
diff --git a/drivers/target/iscsi/iscsi_target_login.c b/drivers/target/iscsi/iscsi_target_login.c
index a3656c9..ae30424 100644
--- a/drivers/target/iscsi/iscsi_target_login.c
+++ b/drivers/target/iscsi/iscsi_target_login.c
@@ -795,22 +795,6 @@ int iscsi_target_setup_login_socket(
 	}
 	np->np_socket = sock;
 	/*
-	 * The SCTP stack needs struct socket->file.
-	 */
-	if ((np->np_network_transport == ISCSI_SCTP_TCP) ||
-	    (np->np_network_transport == ISCSI_SCTP_UDP)) {
-		if (!sock->file) {
-			sock->file = kzalloc(sizeof(struct file), GFP_KERNEL);
-			if (!sock->file) {
-				pr_err("Unable to allocate struct"
-						" file for SCTP\n");
-				ret = -ENOMEM;
-				goto fail;
-			}
-			np->np_flags |= NPF_SCTP_STRUCT_FILE;
-		}
-	}
-	/*
 	 * Setup the np->np_sockaddr from the passed sockaddr setup
 	 * in iscsi_target_configfs.c code..
 	 */
@@ -869,21 +853,15 @@ int iscsi_target_setup_login_socket(
 
 fail:
 	np->np_socket = NULL;
-	if (sock) {
-		if (np->np_flags & NPF_SCTP_STRUCT_FILE) {
-			kfree(sock->file);
-			sock->file = NULL;
-		}
-
+	if (sock)
 		sock_release(sock);
-	}
 	return ret;
 }
 
 static int __iscsi_target_login_thread(struct iscsi_np *np)
 {
 	u8 buffer[ISCSI_HDR_LEN], iscsi_opcode, zero_tsih = 0;
-	int err, ret = 0, set_sctp_conn_flag, stop;
+	int err, ret = 0, stop;
 	struct iscsi_conn *conn = NULL;
 	struct iscsi_login *login;
 	struct iscsi_portal_group *tpg = NULL;
@@ -894,7 +872,6 @@ static int __iscsi_target_login_thread(struct iscsi_np *np)
 	struct sockaddr_in6 sock_in6;
 
 	flush_signals(current);
-	set_sctp_conn_flag = 0;
 	sock = np->np_socket;
 
 	spin_lock_bh(&np->np_thread_lock);
@@ -917,35 +894,12 @@ static int __iscsi_target_login_thread(struct iscsi_np *np)
 		spin_unlock_bh(&np->np_thread_lock);
 		goto out;
 	}
-	/*
-	 * The SCTP stack needs struct socket->file.
-	 */
-	if ((np->np_network_transport == ISCSI_SCTP_TCP) ||
-	    (np->np_network_transport == ISCSI_SCTP_UDP)) {
-		if (!new_sock->file) {
-			new_sock->file = kzalloc(
-					sizeof(struct file), GFP_KERNEL);
-			if (!new_sock->file) {
-				pr_err("Unable to allocate struct"
-						" file for SCTP\n");
-				sock_release(new_sock);
-				/* Get another socket */
-				return 1;
-			}
-			set_sctp_conn_flag = 1;
-		}
-	}
-
 	iscsi_start_login_thread_timer(np);
 
 	conn = kzalloc(sizeof(struct iscsi_conn), GFP_KERNEL);
 	if (!conn) {
 		pr_err("Could not allocate memory for"
 			" new connection\n");
-		if (set_sctp_conn_flag) {
-			kfree(new_sock->file);
-			new_sock->file = NULL;
-		}
 		sock_release(new_sock);
 		/* Get another socket */
 		return 1;
@@ -955,9 +909,6 @@ static int __iscsi_target_login_thread(struct iscsi_np *np)
 	conn->conn_state = TARG_CONN_STATE_FREE;
 	conn->sock = new_sock;
 
-	if (set_sctp_conn_flag)
-		conn->conn_flags |= CONNFLAG_SCTP_STRUCT_FILE;
-
 	pr_debug("Moving to TARG_CONN_STATE_XPT_UP.\n");
 	conn->conn_state = TARG_CONN_STATE_XPT_UP;
 
@@ -1205,13 +1156,8 @@ old_sess_out:
 		iscsi_release_param_list(conn->param_list);
 		conn->param_list = NULL;
 	}
-	if (conn->sock) {
-		if (conn->conn_flags & CONNFLAG_SCTP_STRUCT_FILE) {
-			kfree(conn->sock->file);
-			conn->sock->file = NULL;
-		}
+	if (conn->sock)
 		sock_release(conn->sock);
-	}
 	kfree(conn);
 
 	if (tpg) {

Re: [PATCH] Crash in tun

[Nicholas A. Bellinger]

On Sat, 2012-07-21 at 08:55 +0100, Al Viro wrote:
> 	BTW, speaking of struct file treatment related to sockets -
> there's this piece of code in iscsi:
>         /*
>          * The SCTP stack needs struct socket->file.
>          */
>         if ((np->np_network_transport == ISCSI_SCTP_TCP) ||
>             (np->np_network_transport == ISCSI_SCTP_UDP)) {
>                 if (!new_sock->file) {
>                         new_sock->file = kzalloc(
>                                         sizeof(struct file), GFP_KERNEL);
> 
> For one thing, as far as I can see it'not true - sctp does *not* depend on
> socket->file being non-NULL; it does, in one place, check socket->file->f_flags
> for O_NONBLOCK, but there it treats NULL socket->file as "flag not set".
> Which is the case here anyway - the fake struct file created in
> __iscsi_target_login_thread() (and in iscsi_target_setup_login_socket(), with
> the same excuse) do *not* get that flag set.
> 
> Moreover, it's a bloody serious violation of a bunch of asserts in VFS;
> all struct file instances should come from filp_cachep, via get_empty_filp()
> (or alloc_file(), which is a wrapper for it).  FWIW, I'm very tempted to
> do this and be done with the entire mess:
> 
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---

<nod>  Merged into target-pending.git/for-next.

For the record, this logic was originally required in order to get non
multi-homed SCTP endpoints with iscsi_target_mod to connect using
Core-iSCSI/SCTP initiators to Linux/SCTP, but it's obviously incorrect
for modern code.

Since we don't have iscsi_sctp for Open/iSCSI code implemented just yet
(mnc CC'ed), adding CC' to drop this incorrect code for stable.

Thank you,

--nab

> diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
> index d57d10c..d7dcd67 100644
> --- a/drivers/target/iscsi/iscsi_target.c
> +++ b/drivers/target/iscsi/iscsi_target.c
> @@ -429,19 +429,8 @@ int iscsit_reset_np_thread(
>  
>  int iscsit_del_np_comm(struct iscsi_np *np)
>  {
> -	if (!np->np_socket)
> -		return 0;
> -
> -	/*
> -	 * Some network transports allocate their own struct sock->file,
> -	 * see  if we need to free any additional allocated resources.
> -	 */
> -	if (np->np_flags & NPF_SCTP_STRUCT_FILE) {
> -		kfree(np->np_socket->file);
> -		np->np_socket->file = NULL;
> -	}
> -
> -	sock_release(np->np_socket);
> +	if (np->np_socket)
> +		sock_release(np->np_socket);
>  	return 0;
>  }
>  
> @@ -4077,13 +4066,8 @@ int iscsit_close_connection(
>  	kfree(conn->conn_ops);
>  	conn->conn_ops = NULL;
>  
> -	if (conn->sock) {
> -		if (conn->conn_flags & CONNFLAG_SCTP_STRUCT_FILE) {
> -			kfree(conn->sock->file);
> -			conn->sock->file = NULL;
> -		}
> +	if (conn->sock)
>  		sock_release(conn->sock);
> -	}
>  	conn->thread_set = NULL;
>  
>  	pr_debug("Moving to TARG_CONN_STATE_FREE.\n");
> diff --git a/drivers/target/iscsi/iscsi_target_core.h b/drivers/target/iscsi/iscsi_target_core.h
> index 1c70144..1dd5716 100644
> --- a/drivers/target/iscsi/iscsi_target_core.h
> +++ b/drivers/target/iscsi/iscsi_target_core.h
> @@ -224,7 +224,6 @@ enum iscsi_timer_flags_table {
>  /* Used for struct iscsi_np->np_flags */
>  enum np_flags_table {
>  	NPF_IP_NETWORK		= 0x00,
> -	NPF_SCTP_STRUCT_FILE	= 0x01 /* Bugfix */
>  };
>  
>  /* Used for struct iscsi_np->np_thread_state */
> @@ -503,7 +502,6 @@ struct iscsi_conn {
>  	u16			local_port;
>  	int			net_size;
>  	u32			auth_id;
> -#define CONNFLAG_SCTP_STRUCT_FILE			0x01
>  	u32			conn_flags;
>  	/* Used for iscsi_tx_login_rsp() */
>  	u32			login_itt;
> diff --git a/drivers/target/iscsi/iscsi_target_login.c b/drivers/target/iscsi/iscsi_target_login.c
> index a3656c9..ae30424 100644
> --- a/drivers/target/iscsi/iscsi_target_login.c
> +++ b/drivers/target/iscsi/iscsi_target_login.c
> @@ -795,22 +795,6 @@ int iscsi_target_setup_login_socket(
>  	}
>  	np->np_socket = sock;
>  	/*
> -	 * The SCTP stack needs struct socket->file.
> -	 */
> -	if ((np->np_network_transport == ISCSI_SCTP_TCP) ||
> -	    (np->np_network_transport == ISCSI_SCTP_UDP)) {
> -		if (!sock->file) {
> -			sock->file = kzalloc(sizeof(struct file), GFP_KERNEL);
> -			if (!sock->file) {
> -				pr_err("Unable to allocate struct"
> -						" file for SCTP\n");
> -				ret = -ENOMEM;
> -				goto fail;
> -			}
> -			np->np_flags |= NPF_SCTP_STRUCT_FILE;
> -		}
> -	}
> -	/*
>  	 * Setup the np->np_sockaddr from the passed sockaddr setup
>  	 * in iscsi_target_configfs.c code..
>  	 */
> @@ -869,21 +853,15 @@ int iscsi_target_setup_login_socket(
>  
>  fail:
>  	np->np_socket = NULL;
> -	if (sock) {
> -		if (np->np_flags & NPF_SCTP_STRUCT_FILE) {
> -			kfree(sock->file);
> -			sock->file = NULL;
> -		}
> -
> +	if (sock)
>  		sock_release(sock);
> -	}
>  	return ret;
>  }
>  
>  static int __iscsi_target_login_thread(struct iscsi_np *np)
>  {
>  	u8 buffer[ISCSI_HDR_LEN], iscsi_opcode, zero_tsih = 0;
> -	int err, ret = 0, set_sctp_conn_flag, stop;
> +	int err, ret = 0, stop;
>  	struct iscsi_conn *conn = NULL;
>  	struct iscsi_login *login;
>  	struct iscsi_portal_group *tpg = NULL;
> @@ -894,7 +872,6 @@ static int __iscsi_target_login_thread(struct iscsi_np *np)
>  	struct sockaddr_in6 sock_in6;
>  
>  	flush_signals(current);
> -	set_sctp_conn_flag = 0;
>  	sock = np->np_socket;
>  
>  	spin_lock_bh(&np->np_thread_lock);
> @@ -917,35 +894,12 @@ static int __iscsi_target_login_thread(struct iscsi_np *np)
>  		spin_unlock_bh(&np->np_thread_lock);
>  		goto out;
>  	}
> -	/*
> -	 * The SCTP stack needs struct socket->file.
> -	 */
> -	if ((np->np_network_transport == ISCSI_SCTP_TCP) ||
> -	    (np->np_network_transport == ISCSI_SCTP_UDP)) {
> -		if (!new_sock->file) {
> -			new_sock->file = kzalloc(
> -					sizeof(struct file), GFP_KERNEL);
> -			if (!new_sock->file) {
> -				pr_err("Unable to allocate struct"
> -						" file for SCTP\n");
> -				sock_release(new_sock);
> -				/* Get another socket */
> -				return 1;
> -			}
> -			set_sctp_conn_flag = 1;
> -		}
> -	}
> -
>  	iscsi_start_login_thread_timer(np);
>  
>  	conn = kzalloc(sizeof(struct iscsi_conn), GFP_KERNEL);
>  	if (!conn) {
>  		pr_err("Could not allocate memory for"
>  			" new connection\n");
> -		if (set_sctp_conn_flag) {
> -			kfree(new_sock->file);
> -			new_sock->file = NULL;
> -		}
>  		sock_release(new_sock);
>  		/* Get another socket */
>  		return 1;
> @@ -955,9 +909,6 @@ static int __iscsi_target_login_thread(struct iscsi_np *np)
>  	conn->conn_state = TARG_CONN_STATE_FREE;
>  	conn->sock = new_sock;
>  
> -	if (set_sctp_conn_flag)
> -		conn->conn_flags |= CONNFLAG_SCTP_STRUCT_FILE;
> -
>  	pr_debug("Moving to TARG_CONN_STATE_XPT_UP.\n");
>  	conn->conn_state = TARG_CONN_STATE_XPT_UP;
>  
> @@ -1205,13 +1156,8 @@ old_sess_out:
>  		iscsi_release_param_list(conn->param_list);
>  		conn->param_list = NULL;
>  	}
> -	if (conn->sock) {
> -		if (conn->conn_flags & CONNFLAG_SCTP_STRUCT_FILE) {
> -			kfree(conn->sock->file);
> -			conn->sock->file = NULL;
> -		}
> +	if (conn->sock)
>  		sock_release(conn->sock);
> -	}
>  	kfree(conn);
>  
>  	if (tpg) {

RE: [PATCH] Crash in tun

[David Laight]

> 	BTW, speaking of struct file treatment related to sockets -
> there's this piece of code in iscsi:
>         /*
>          * The SCTP stack needs struct socket->file.
>          */
>         if ((np->np_network_transport == ISCSI_SCTP_TCP) ||
>             (np->np_network_transport == ISCSI_SCTP_UDP)) {
>                 if (!new_sock->file) {
>                         new_sock->file = kzalloc(
>                                         sizeof(struct file),
> GFP_KERNEL);
> 
> For one thing, as far as I can see it'not true - sctp does *not*
> depend on socket->file being non-NULL; it does, in one place,
> check socket->file->f_flags for O_NONBLOCK, but there it treats
> NULL socket->file as "flag no set".

The SCTP code certainly has unconditionally looked at file->f_flags,
we had to allocate a 'struct file' for our in-kernel socket code.
We set sock->file = NULL before the sock_release() call so
hopefully don't suffer the 'side effects'.

	David