[PATCH] [ATM]: Add error handling for proc_create fail

[PATCH] [ATM]: Add error handling for proc_create fail

[Wang Chen]

When proc_create() fail, we return -ENOMEM.

Signed-off-by: Wang Chen <wangchen@cn.fujitsu.com>
---
 net/atm/clip.c |    4 ++++
 net/atm/lec.c  |    4 ++++
 2 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/net/atm/clip.c b/net/atm/clip.c
index d30167c..57ed448 100644
--- a/net/atm/clip.c
+++ b/net/atm/clip.c
@@ -963,6 +963,10 @@ static int __init atm_clip_init(void)
 		struct proc_dir_entry *p;
 
 		p = proc_create("arp", S_IRUGO, atm_proc_root, &arp_seq_fops);
+		if (!p) {
+			printk(KERN_ERR "Unable to initialize /proc/atm/arp\n");
+			return -ENOMEM;
+		}
 	}
 #endif
 
diff --git a/net/atm/lec.c b/net/atm/lec.c
index 0e450d1..532965d 100644
--- a/net/atm/lec.c
+++ b/net/atm/lec.c
@@ -1250,6 +1250,10 @@ static int __init lane_module_init(void)
 	struct proc_dir_entry *p;
 
 	p = proc_create("lec", S_IRUGO, atm_proc_root, &lec_seq_fops);
+	if (!p) {
+		printk(KERN_ERR "Unable to initialize /proc/atm/lec\n");
+		return -ENOMEM;
+	}
 #endif
 
 	register_atm_ioctl(&lane_ioctl_ops);
-- 
WCN

Re: [PATCH] [ATM]: Add error handling for proc_create fail

[chas williams - CONTRACTOR]

it should be /proc/net/atm/foo not /proc/atm/foo i think.

In message <47C7B79C.2010304@cn.fujitsu.com>,Wang Chen writes:
>When proc_create() fail, we return -ENOMEM.
>
>Signed-off-by: Wang Chen <wangchen@cn.fujitsu.com>
>---
> net/atm/clip.c |    4 ++++
> net/atm/lec.c  |    4 ++++
> 2 files changed, 8 insertions(+), 0 deletions(-)
>
>diff --git a/net/atm/clip.c b/net/atm/clip.c
>index d30167c..57ed448 100644
>--- a/net/atm/clip.c
>+++ b/net/atm/clip.c
>@@ -963,6 +963,10 @@ static int __init atm_clip_init(void)
> 		struct proc_dir_entry *p;
> 
> 		p = proc_create("arp", S_IRUGO, atm_proc_root, &arp_seq_fops);
>+		if (!p) {
>+			printk(KERN_ERR "Unable to initialize /proc/atm/arp\n");
>+			return -ENOMEM;
>+		}
> 	}
> #endif
> 
>diff --git a/net/atm/lec.c b/net/atm/lec.c
>index 0e450d1..532965d 100644
>--- a/net/atm/lec.c
>+++ b/net/atm/lec.c
>@@ -1250,6 +1250,10 @@ static int __init lane_module_init(void)
> 	struct proc_dir_entry *p;
> 
> 	p = proc_create("lec", S_IRUGO, atm_proc_root, &lec_seq_fops);
>+	if (!p) {
>+		printk(KERN_ERR "Unable to initialize /proc/atm/lec\n");
>+		return -ENOMEM;
>+	}
> #endif
> 
> 	register_atm_ioctl(&lane_ioctl_ops);

Re: [PATCH] [ATM]: Add error handling for proc_create fail

[David Miller]

From: Wang Chen <wangchen@cn.fujitsu.com>
Date: Fri, 29 Feb 2008 15:43:24 +0800

> @@ -963,6 +963,10 @@ static int __init atm_clip_init(void)
>  		struct proc_dir_entry *p;
>  
>  		p = proc_create("arp", S_IRUGO, atm_proc_root, &arp_seq_fops);
> +		if (!p) {
> +			printk(KERN_ERR "Unable to initialize /proc/atm/arp\n");
> +			return -ENOMEM;
> +		}
>  	}
>  #endif
>  

This is not sufficient.  If this fails you need to unregister
the ioctl and all of the other things that get registered
before this procfs registry runs.

take#2: [PATCH] [ATM]: Add error handling for proc_create fail

[Wang Chen]

David Miller said the following on 2008-3-1 2:38:
> This is not sufficient.  If this fails you need to unregister
> the ioctl and all of the other things that get registered
> before this procfs registry runs.
> 

When proc_create() fail, we return -ENOMEM.

Signed-off-by: Wang Chen <wangchen@cn.fujitsu.com>
---
 net/atm/clip.c |   18 ++++++++++--------
 net/atm/lec.c  |    4 ++++
 2 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/net/atm/clip.c b/net/atm/clip.c
index d30167c..66e5a91 100644
--- a/net/atm/clip.c
+++ b/net/atm/clip.c
@@ -949,6 +949,16 @@ static const struct file_operations arp_seq_fops = {
 
 static int __init atm_clip_init(void)
 {
+#ifdef CONFIG_PROC_FS
+	struct proc_dir_entry *p;
+
+	p = proc_create("arp", S_IRUGO, atm_proc_root, &arp_seq_fops);
+	if (!p) {
+		printk(KERN_ERR "Unable to initialize /proc/net/atm/arp\n");
+		return -ENOMEM;
+	}
+#endif
+
 	neigh_table_init_no_netlink(&clip_tbl);
 
 	clip_tbl_hook = &clip_tbl;
@@ -958,14 +968,6 @@ static int __init atm_clip_init(void)
 
 	setup_timer(&idle_timer, idle_timer_check, 0);
 
-#ifdef CONFIG_PROC_FS
-	{
-		struct proc_dir_entry *p;
-
-		p = proc_create("arp", S_IRUGO, atm_proc_root, &arp_seq_fops);
-	}
-#endif
-
 	return 0;
 }
 
diff --git a/net/atm/lec.c b/net/atm/lec.c
index 0e450d1..a2efa7f 100644
--- a/net/atm/lec.c
+++ b/net/atm/lec.c
@@ -1250,6 +1250,10 @@ static int __init lane_module_init(void)
 	struct proc_dir_entry *p;
 
 	p = proc_create("lec", S_IRUGO, atm_proc_root, &lec_seq_fops);
+	if (!p) {
+		printk(KERN_ERR "Unable to initialize /proc/net/atm/lec\n");
+		return -ENOMEM;
+	}
 #endif
 
 	register_atm_ioctl(&lane_ioctl_ops);
-- 
WCN

Re: take#2: [PATCH] [ATM]: Add error handling for proc_create fail

[David Miller]

From: Wang Chen <wangchen@cn.fujitsu.com>
Date: Mon, 03 Mar 2008 10:44:33 +0800

> David Miller said the following on 2008-3-1 2:38:
> > This is not sufficient.  If this fails you need to unregister
> > the ioctl and all of the other things that get registered
> > before this procfs registry runs.
> > 
> 
> When proc_create() fail, we return -ENOMEM.
> 
> Signed-off-by: Wang Chen <wangchen@cn.fujitsu.com>

Sigh....

I know you want to avoid all of the error handling unwind code, but
you can't do that in this situation.  It's not valid to register the
ATM clip procfs file without first initializing the ATM clip neighbour
table, so moving it up like this isn't going to work.

They way you've done it, a user can open the procfs file and access
the datastructure before it's initialized properly.

Please, simply add the necessary unregister code and leave the procfs
initialization where it currently is.

I get the sense you really aren't reading this code carefully and
you're tossing these patches together as quickly as you can to just
get it done.  Please don't do that, it wastes both my time and
your's.

Thank you.

take#3: [PATCH] [ATM]: Add error handling for proc_create fail

[Wang Chen]

David Miller said the following on 2008-3-3 13:09:
> Sigh....
> 
> I know you want to avoid all of the error handling unwind code, but
> you can't do that in this situation.  It's not valid to register the
> ATM clip procfs file without first initializing the ATM clip neighbour
> table, so moving it up like this isn't going to work.
> 
> They way you've done it, a user can open the procfs file and access
> the datastructure before it's initialized properly.
> 
> Please, simply add the necessary unregister code and leave the procfs
> initialization where it currently is.
> 
> I get the sense you really aren't reading this code carefully and
> you're tossing these patches together as quickly as you can to just
> get it done.  Please don't do that, it wastes both my time and
> your's.
> 

No, I didn't read the code very carefully. Sorry for that.
However, I don't think it's necessary to put procfs init after ioctl,
although it's necessary to do clip procfs init after neighbour tbl init.
If i am wrong, please correct me.

But I still put clip profs init where it was, because all the unregister
can be done easily when I pick the code up to a static function called
atm_clip_exit_noproc().

Here is the new patch.

---
When proc_create() fails, do some error handling work and return -ENOMEM.

Signed-off-by: Wang Chen <wangchen@cn.fujitsu.com>
---
 net/atm/clip.c |   61 +++++++++++++++++++++++++++++++++-----------------------
 net/atm/lec.c  |    4 +++
 2 files changed, 40 insertions(+), 25 deletions(-)

diff --git a/net/atm/clip.c b/net/atm/clip.c
index d30167c..24c60ba 100644
--- a/net/atm/clip.c
+++ b/net/atm/clip.c
@@ -947,34 +947,10 @@ static const struct file_operations arp_seq_fops = {
 };
 #endif
 
-static int __init atm_clip_init(void)
-{
-	neigh_table_init_no_netlink(&clip_tbl);
-
-	clip_tbl_hook = &clip_tbl;
-	register_atm_ioctl(&clip_ioctl_ops);
-	register_netdevice_notifier(&clip_dev_notifier);
-	register_inetaddr_notifier(&clip_inet_notifier);
-
-	setup_timer(&idle_timer, idle_timer_check, 0);
-
-#ifdef CONFIG_PROC_FS
-	{
-		struct proc_dir_entry *p;
-
-		p = proc_create("arp", S_IRUGO, atm_proc_root, &arp_seq_fops);
-	}
-#endif
-
-	return 0;
-}
-
-static void __exit atm_clip_exit(void)
+static void atm_clip_exit_noproc(void)
 {
 	struct net_device *dev, *next;
 
-	remove_proc_entry("arp", atm_proc_root);
-
 	unregister_inetaddr_notifier(&clip_inet_notifier);
 	unregister_netdevice_notifier(&clip_dev_notifier);
 
@@ -1005,6 +981,41 @@ static void __exit atm_clip_exit(void)
 	clip_tbl_hook = NULL;
 }
 
+static int __init atm_clip_init(void)
+{
+	neigh_table_init_no_netlink(&clip_tbl);
+
+	clip_tbl_hook = &clip_tbl;
+	register_atm_ioctl(&clip_ioctl_ops);
+	register_netdevice_notifier(&clip_dev_notifier);
+	register_inetaddr_notifier(&clip_inet_notifier);
+
+	setup_timer(&idle_timer, idle_timer_check, 0);
+
+#ifdef CONFIG_PROC_FS
+	{
+		struct proc_dir_entry *p;
+
+		p = proc_create("arp", S_IRUGO, atm_proc_root, &arp_seq_fops);
+		if (!p) {
+			printk(KERN_ERR "Unable to initialize "
+			       "/proc/net/atm/arp\n");
+			atm_clip_exit_noproc();
+			return -ENOMEM;
+		}
+	}
+#endif
+
+	return 0;
+}
+
+static void __exit atm_clip_exit(void)
+{
+	remove_proc_entry("arp", atm_proc_root);
+
+	atm_clip_exit_noproc();
+}
+
 module_init(atm_clip_init);
 module_exit(atm_clip_exit);
 MODULE_AUTHOR("Werner Almesberger");
diff --git a/net/atm/lec.c b/net/atm/lec.c
index 0e450d1..a2efa7f 100644
--- a/net/atm/lec.c
+++ b/net/atm/lec.c
@@ -1250,6 +1250,10 @@ static int __init lane_module_init(void)
 	struct proc_dir_entry *p;
 
 	p = proc_create("lec", S_IRUGO, atm_proc_root, &lec_seq_fops);
+	if (!p) {
+		printk(KERN_ERR "Unable to initialize /proc/net/atm/lec\n");
+		return -ENOMEM;
+	}
 #endif
 
 	register_atm_ioctl(&lane_ioctl_ops);
-- 
WCN

take#4: [PATCH] [ATM]: Add error handling for proc_create fail

[Wang Chen]

Wang Chen said the following on 2008-3-3 16:46:
> David Miller said the following on 2008-3-3 13:09:
>> Sigh....
>>
>> I know you want to avoid all of the error handling unwind code, but
>> you can't do that in this situation.  It's not valid to register the
>> ATM clip procfs file without first initializing the ATM clip neighbour
>> table, so moving it up like this isn't going to work.
>>
>> They way you've done it, a user can open the procfs file and access
>> the datastructure before it's initialized properly.
>>
>> Please, simply add the necessary unregister code and leave the procfs
>> initialization where it currently is.
>>
>> I get the sense you really aren't reading this code carefully and
>> you're tossing these patches together as quickly as you can to just
>> get it done.  Please don't do that, it wastes both my time and
>> your's.
>>
> 
> No, I didn't read the code very carefully. Sorry for that.
> However, I don't think it's necessary to put procfs init after ioctl,
> although it's necessary to do clip procfs init after neighbour tbl init.
> If i am wrong, please correct me.
> 
> But I still put clip profs init where it was, because all the unregister
> can be done easily when I pick the code up to a static function called
> atm_clip_exit_noproc().
> 
> Here is the new patch.
> 

Hi, David, please ignore the previous patch. It's ugly and difficult to
understand.

Here is a new one.

---
When proc_create() fails, do some error handling work and return -ENOMEM.

Signed-off-by: Wang Chen <wangchen@cn.fujitsu.com>
---
 net/atm/clip.c |   19 ++++++++++++++++---
 net/atm/lec.c  |    4 ++++
 2 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/net/atm/clip.c b/net/atm/clip.c
index d30167c..2ab1e36 100644
--- a/net/atm/clip.c
+++ b/net/atm/clip.c
@@ -947,6 +947,8 @@ static const struct file_operations arp_seq_fops = {
 };
 #endif
 
+static void atm_clip_exit_noproc(void);
+
 static int __init atm_clip_init(void)
 {
 	neigh_table_init_no_netlink(&clip_tbl);
@@ -963,18 +965,22 @@ static int __init atm_clip_init(void)
 		struct proc_dir_entry *p;
 
 		p = proc_create("arp", S_IRUGO, atm_proc_root, &arp_seq_fops);
+		if (!p) {
+			printk(KERN_ERR "Unable to initialize "
+			       "/proc/net/atm/arp\n");
+			atm_clip_exit_noproc();
+			return -ENOMEM;
+		}
 	}
 #endif
 
 	return 0;
 }
 
-static void __exit atm_clip_exit(void)
+static void atm_clip_exit_noproc(void)
 {
 	struct net_device *dev, *next;
 
-	remove_proc_entry("arp", atm_proc_root);
-
 	unregister_inetaddr_notifier(&clip_inet_notifier);
 	unregister_netdevice_notifier(&clip_dev_notifier);
 
@@ -1005,6 +1011,13 @@ static void __exit atm_clip_exit(void)
 	clip_tbl_hook = NULL;
 }
 
+static void __exit atm_clip_exit(void)
+{
+	remove_proc_entry("arp", atm_proc_root);
+
+	atm_clip_exit_noproc();
+}
+
 module_init(atm_clip_init);
 module_exit(atm_clip_exit);
 MODULE_AUTHOR("Werner Almesberger");
diff --git a/net/atm/lec.c b/net/atm/lec.c
index 0e450d1..a2efa7f 100644
--- a/net/atm/lec.c
+++ b/net/atm/lec.c
@@ -1250,6 +1250,10 @@ static int __init lane_module_init(void)
 	struct proc_dir_entry *p;
 
 	p = proc_create("lec", S_IRUGO, atm_proc_root, &lec_seq_fops);
+	if (!p) {
+		printk(KERN_ERR "Unable to initialize /proc/net/atm/lec\n");
+		return -ENOMEM;
+	}
 #endif
 
 	register_atm_ioctl(&lane_ioctl_ops);
-- 
WCN

Re: take#4: [PATCH] [ATM]: Add error handling for proc_create fail

[David Miller]

From: Wang Chen <wangchen@cn.fujitsu.com>
Date: Tue, 04 Mar 2008 15:27:41 +0800

> When proc_create() fails, do some error handling work and return -ENOMEM.
> 
> Signed-off-by: Wang Chen <wangchen@cn.fujitsu.com>

Applied, thanks a lot.