| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20080428153716.GC7334@c2.user-mode-linux.org>
Date: Mon, 28 Apr 2008 11:37:16 -0400
From: Jeff Dike <jdike@...toit.com>
To: WANG Cong <xiyou.wangcong@...il.com>
Cc: akpm@...l.org, linux-kernel@...r.kernel.org,
user-mode-linux-devel@...ts.sourceforge.net
Subject: Re: [PATCH 7/19] UML - Move hppfs_kern.c to hppfs.c
On Sat, Apr 26, 2008 at 05:17:55PM +0800, WANG Cong wrote:
> > +static struct inode *get_inode(struct super_block *, struct dentry *);
> > +
> > +struct hppfs_data {
> > + struct list_head list;
> > + char contents[PAGE_SIZE - sizeof(struct list_head)];
> > +};
> > +
> > +struct hppfs_private {
> > + struct file *proc_file;
> > + int host_fd;
> > + loff_t len;
> > + struct hppfs_data *contents;
> > +};
> > +
> > +struct hppfs_inode_info {
> > + struct dentry *proc_dentry;
> > + struct inode vfs_inode;
> > +};
> > +
> > +static inline struct hppfs_inode_info *HPPFS_I(struct inode *inode)
> > +{
> > + return container_of(inode, struct hppfs_inode_info, vfs_inode);
> > +}
> > +
> > +#define HPPFS_SUPER_MAGIC 0xb00000ee
>
>
> These can be put into a single header, e.g. hppfs.h.
Why, when this one C file is the only user?
> > +static char *dentry_name(struct dentry *dentry, int extra)
> > +{
> > + struct dentry *parent;
> > + char *root, *name;
> > + const char *seg_name;
> > + int len, seg_len;
> > +
> > + len = 0;
> > + parent = dentry;
>
> These can be put into initialization, I think, that's more readable.
Yup.
> > + if (fd > 0) {
> > + os_close_file(fd);
> > + return 1;
> > + }
>
>
> How about 'fd < 0' ?
OK, that would remove the braces at least...
> > + new = kmalloc(sizeof(*data), GFP_KERNEL);
> > + if (new == 0) {
>
>
> Using NULL is better, at least sparse complains about this. ;)
Good, I never liked using 0 (or !ptr) in place of NULL anyway.
> > +static int hppfs_filldir(void *d, const char *name, int size,
> > + loff_t offset, u64 inode, unsigned int type)
> > +{
> > + struct hppfs_dirent *dirent = d;
> > +
> > + if (file_removed(dirent->dentry, name))
> > + return 0;
>
> If returning 0 indicates success, this is wrong, since file_removed() may
> return -ENOMEM.
>
> Or I miss something that is too obvious? ;-)
Oops, nice spotting.
See what you think about the patch below...
Jeff
--
Work email - jdike at linux dot intel dot com
Index: linux-2.6.22/fs/hppfs/hppfs.c
===================================================================
--- linux-2.6.22.orig/fs/hppfs/hppfs.c 2008-04-28 11:21:38.000000000 -0400
+++ linux-2.6.22/fs/hppfs/hppfs.c 2008-04-28 11:34:58.000000000 -0400
@@ -64,13 +64,11 @@ static int is_pid(struct dentry *dentry)
static char *dentry_name(struct dentry *dentry, int extra)
{
- struct dentry *parent;
- char *root, *name;
+ struct dentry *parent = dentry;
+ char *root = "proc", *name;
const char *seg_name;
- int len, seg_len;
+ int len = 0, seg_len;
- len = 0;
- parent = dentry;
while (parent->d_parent != parent) {
if (is_pid(parent))
len += strlen("pid") + 1;
@@ -78,7 +76,6 @@ static char *dentry_name(struct dentry *
parent = parent->d_parent;
}
- root = "proc";
len += strlen(root);
name = kmalloc(len + extra + 1, GFP_KERNEL);
if (name == NULL)
@@ -128,11 +125,11 @@ static int file_removed(struct dentry *d
fd = os_open_file(host_file, of_read(OPENFLAGS()), 0);
kfree(host_file);
- if (fd > 0) {
- os_close_file(fd);
- return 1;
- }
- return 0;
+ if (fd < 0)
+ return 0;
+
+ os_close_file(fd);
+ return 1;
}
static struct dentry *hppfs_lookup(struct inode *ino, struct dentry *dentry,
@@ -210,11 +207,10 @@ static ssize_t read_proc(struct file *fi
static ssize_t hppfs_read_file(int fd, char __user *buf, ssize_t count)
{
- ssize_t n;
+ ssize_t n = -ENOMEM;
int cur, err;
char *new_buf;
- n = -ENOMEM;
new_buf = kmalloc(PAGE_SIZE, GFP_KERNEL);
if (new_buf == NULL) {
printk(KERN_ERR "hppfs_read_file : kmalloc failed\n");
@@ -271,8 +267,10 @@ static ssize_t hppfs_read(struct file *f
count = hppfs->len - off;
rem = copy_to_user(buf, &data->contents[off], count);
*ppos += count - rem;
- if (rem > 0)
+ if (rem == count)
return -EFAULT;
+ else if (rem > 0)
+ return count - rem;
} else if (hppfs->host_fd != -1) {
err = os_seek_file(hppfs->host_fd, *ppos);
if (err) {
@@ -380,7 +378,7 @@ static struct hppfs_data *hppfs_get_data
break;
new = kmalloc(sizeof(*data), GFP_KERNEL);
- if (new == 0) {
+ if (new == NULL) {
printk(KERN_ERR "hppfs_get_data : data allocation "
"failed\n");
err = -ENOMEM;
@@ -551,8 +549,11 @@ static int hppfs_filldir(void *d, const
loff_t offset, u64 inode, unsigned int type)
{
struct hppfs_dirent *dirent = d;
+ int gone = file_removed(dirent->dentry, name);
- if (file_removed(dirent->dentry, name))
+ if (gone < 0)
+ return gone;
+ else if (gone)
return 0;
return (*dirent->filldir)(dirent->vfs_dirent, name, size, offset,
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists