2004-11-02 08:47:33

by Milton Miller

[permalink] [raw]
Subject: sysfs backing store error path confusion

sysfs_new_dirent returns ERR_PTR(-ENOMEM) if kmalloc fails but the callers
were expecting NULL.

Compile & link tested. (Yes, changing the callee would be a smaller change).

===== fs/sysfs/dir.c 1.24 vs edited =====
--- 1.24/fs/sysfs/dir.c 2004-11-01 21:46:46 +01:00
+++ edited/fs/sysfs/dir.c 2004-11-02 09:12:31 +01:00
@@ -55,8 +55,8 @@ int sysfs_make_dirent(struct sysfs_diren
struct sysfs_dirent * sd;

sd = sysfs_new_dirent(parent_sd, element);
- if (!sd)
- return -ENOMEM;
+ if (IS_ERR(sd))
+ return PTR_ERR(sd);

sd->s_mode = mode;
sd->s_type = type;
@@ -332,13 +332,18 @@ static int sysfs_dir_open(struct inode *
{
struct dentry * dentry = file->f_dentry;
struct sysfs_dirent * parent_sd = dentry->d_fsdata;
+ struct sysfs_dirent * sd;

down(&dentry->d_inode->i_sem);
- file->private_data = sysfs_new_dirent(parent_sd, NULL);
+ sd = sysfs_new_dirent(parent_sd, NULL);
up(&dentry->d_inode->i_sem);

- return file->private_data ? 0 : -ENOMEM;
+ if (IS_ERR(sd))
+ return PTR_ERR(sd);
+
+ file->private_data = sd;

+ return 0;
}

static int sysfs_dir_close(struct inode *inode, struct file *file)


2004-11-02 16:08:56

by Maneesh Soni

[permalink] [raw]
Subject: Re: sysfs backing store error path confusion

On Tue, Nov 02, 2004 at 02:46:58AM -0600, Milton Miller wrote:
> sysfs_new_dirent returns ERR_PTR(-ENOMEM) if kmalloc fails but the callers
> were expecting NULL.
>
> Compile & link tested. (Yes, changing the callee would be a smaller change).
>

Thanks for spotting this. But as you said, I will prefer to change the callee.
How about this patch?

Andrew, Greg, please include this if found ok.

Thanks
Maneesh



o sysfs_new_dirent to retrun NULL if kmalloc fails. Thanks to Milton Miller
for spotting this.

Signed-off-by: <[email protected]>
---

linux-2.6.10-rc1-bk12-maneesh/fs/sysfs/dir.c | 2 +-
1 files changed, 1 insertion(+), 1 deletion(-)

diff -puN fs/sysfs/dir.c~fix-sysfs_new_dirent-return fs/sysfs/dir.c
--- linux-2.6.10-rc1-bk12/fs/sysfs/dir.c~fix-sysfs_new_dirent-return 2004-11-02 08:38:57.000000000 -0600
+++ linux-2.6.10-rc1-bk12-maneesh/fs/sysfs/dir.c 2004-11-02 09:17:18.000000000 -0600
@@ -38,7 +38,7 @@ static struct sysfs_dirent * sysfs_new_d

sd = kmalloc(sizeof(*sd), GFP_KERNEL);
if (!sd)
- return -ENOMEM;
+ return NULL;

memset(sd, 0, sizeof(*sd));
atomic_set(&sd->s_count, 1);
_

--
Maneesh Soni
Linux Technology Center,
IBM Austin
email: [email protected]
Phone: 1-512-838-1896 Fax:
T/L : 6781896

2004-11-03 21:52:31

by Greg KH

[permalink] [raw]
Subject: Re: sysfs backing store error path confusion

On Tue, Nov 02, 2004 at 10:03:34AM -0600, Maneesh Soni wrote:
> On Tue, Nov 02, 2004 at 02:46:58AM -0600, Milton Miller wrote:
> > sysfs_new_dirent returns ERR_PTR(-ENOMEM) if kmalloc fails but the callers
> > were expecting NULL.
> >
> > Compile & link tested. (Yes, changing the callee would be a smaller change).
> >
>
> Thanks for spotting this. But as you said, I will prefer to change the callee.
> How about this patch?
>
> Andrew, Greg, please include this if found ok.
>
> Thanks
> Maneesh
>
>
>
> o sysfs_new_dirent to retrun NULL if kmalloc fails. Thanks to Milton Miller
> for spotting this.
>
> Signed-off-by: <[email protected]>
> ---
>
> linux-2.6.10-rc1-bk12-maneesh/fs/sysfs/dir.c | 2 +-
> 1 files changed, 1 insertion(+), 1 deletion(-)
>
> diff -puN fs/sysfs/dir.c~fix-sysfs_new_dirent-return fs/sysfs/dir.c
> --- linux-2.6.10-rc1-bk12/fs/sysfs/dir.c~fix-sysfs_new_dirent-return 2004-11-02 08:38:57.000000000 -0600
> +++ linux-2.6.10-rc1-bk12-maneesh/fs/sysfs/dir.c 2004-11-02 09:17:18.000000000 -0600
> @@ -38,7 +38,7 @@ static struct sysfs_dirent * sysfs_new_d
>
> sd = kmalloc(sizeof(*sd), GFP_KERNEL);
> if (!sd)
> - return -ENOMEM;
> + return NULL;

Actually, this needs to be a 0, not NULL, otherwise the compiler
complains with a warning. I've fixed it up and applied it.

thanks,

greg k-h

2004-11-05 07:49:44

by Milton Miller

[permalink] [raw]
Subject: Re: sysfs backing store error path confusion


On Nov 3, 2004, at 3:42 PM, Greg KH wrote:

|On Tue, Nov 02, 2004 at 10:03:34AM -0600, Maneesh Soni wrote:
||On Tue, Nov 02, 2004 at 02:46:58AM -0600, Milton Miller wrote:
|||sysfs_new_dirent returns ERR_PTR(-ENOMEM) if kmalloc fails but the callers
|||were expecting NULL.
||
||Thanks for spotting this. But as you said, I will prefer to change the callee.
||How about this patch?
..
||- return -ENOMEM;
||+ return NULL;
|
|Actually, this needs to be a 0, not NULL, otherwise the compiler
|complains with a warning. I've fixed it up and applied it.
|
|thanks,
|
|greg k-h

I wondered why greg thought the type was wrong. After it was merged I
realized that the wrong function was changed. Here's an attempt to fix
both errors.

milton

===== fs/sysfs/dir.c 1.27 vs edited =====
--- 1.27/fs/sysfs/dir.c 2004-11-04 22:37:32 +01:00
+++ edited/fs/sysfs/dir.c 2004-11-05 08:10:54 +01:00
@@ -38,7 +38,7 @@ static struct sysfs_dirent * sysfs_new_d

sd = kmalloc(sizeof(*sd), GFP_KERNEL);
if (!sd)
- return ERR_PTR(-ENOMEM);
+ return NULL;

memset(sd, 0, sizeof(*sd));
atomic_set(&sd->s_count, 1);
@@ -56,7 +56,7 @@ int sysfs_make_dirent(struct sysfs_diren

sd = sysfs_new_dirent(parent_sd, element);
if (!sd)
- return 0;
+ return -ENOMEM;

sd->s_mode = mode;
sd->s_type = type;

2004-11-06 06:20:45

by Maneesh Soni

[permalink] [raw]
Subject: Re: sysfs backing store error path confusion

On Fri, Nov 05, 2004 at 01:49:34AM -0600, Milton Miller wrote:
>
> On Nov 3, 2004, at 3:42 PM, Greg KH wrote:
>
> |On Tue, Nov 02, 2004 at 10:03:34AM -0600, Maneesh Soni wrote:
> ||On Tue, Nov 02, 2004 at 02:46:58AM -0600, Milton Miller wrote:
> |||sysfs_new_dirent returns ERR_PTR(-ENOMEM) if kmalloc fails but the callers
> |||were expecting NULL.
> ||
> ||Thanks for spotting this. But as you said, I will prefer to change the callee.
> ||How about this patch?
> ..
> ||- return -ENOMEM;
> ||+ return NULL;
> |
> |Actually, this needs to be a 0, not NULL, otherwise the compiler
> |complains with a warning. I've fixed it up and applied it.
> |
> |thanks,
> |
> |greg k-h
>
> I wondered why greg thought the type was wrong. After it was merged I
> realized that the wrong function was changed. Here's an attempt to fix
> both errors.
>

Yes, it is correct now. Sorry about the confusion. I edited at the wrong
palce.

Thanks
Maneesh

>
> ===== fs/sysfs/dir.c 1.27 vs edited =====
> --- 1.27/fs/sysfs/dir.c 2004-11-04 22:37:32 +01:00
> +++ edited/fs/sysfs/dir.c 2004-11-05 08:10:54 +01:00
> @@ -38,7 +38,7 @@ static struct sysfs_dirent * sysfs_new_d
>
> sd = kmalloc(sizeof(*sd), GFP_KERNEL);
> if (!sd)
> - return ERR_PTR(-ENOMEM);
> + return NULL;
>
> memset(sd, 0, sizeof(*sd));
> atomic_set(&sd->s_count, 1);
> @@ -56,7 +56,7 @@ int sysfs_make_dirent(struct sysfs_diren
>
> sd = sysfs_new_dirent(parent_sd, element);
> if (!sd)
> - return 0;
> + return -ENOMEM;
>
> sd->s_mode = mode;
> sd->s_type = type;
>

--
Maneesh Soni
Linux Technology Center,
IBM Austin
email: [email protected]
Phone: 1-512-838-1896 Fax:
T/L : 6781896

2004-11-12 21:31:51

by Greg KH

[permalink] [raw]
Subject: Re: sysfs backing store error path confusion

On Fri, Nov 05, 2004 at 01:49:34AM -0600, Milton Miller wrote:
>
> On Nov 3, 2004, at 3:42 PM, Greg KH wrote:
>
> |On Tue, Nov 02, 2004 at 10:03:34AM -0600, Maneesh Soni wrote:
> ||On Tue, Nov 02, 2004 at 02:46:58AM -0600, Milton Miller wrote:
> |||sysfs_new_dirent returns ERR_PTR(-ENOMEM) if kmalloc fails but the callers
> |||were expecting NULL.
> ||
> ||Thanks for spotting this. But as you said, I will prefer to change the callee.
> ||How about this patch?
> ..
> ||- return -ENOMEM;
> ||+ return NULL;
> |
> |Actually, this needs to be a 0, not NULL, otherwise the compiler
> |complains with a warning. I've fixed it up and applied it.
> |
> |thanks,
> |
> |greg k-h
>
> I wondered why greg thought the type was wrong. After it was merged I
> realized that the wrong function was changed. Here's an attempt to fix
> both errors.
>
> milton

Applied, thanks.

greg k-h