2003-03-06 22:37:35

by sudharsan vijayaraghavan

[permalink] [raw]
Subject: fd_install question ??

Hi,

We modified the existing /usr/src/linux/fs/pipe.c such that we
need more than 2 file descriptors where one of the descriptors
will be writer and others should be readers.

However while doing an fd_install on the third descriptor we
encounter an "OOPS" .

The peculair thing is we are able to successfully read using the
third descriptor , while our program exits we encounter the
"OOPS".

We modified the code as follows :

I have highlighted modifications using arrows ( --> ):
------------------------------------------------------

We made fd[0] as write end and fd[1],fd[2] as read ends.
We also successfully received three file descriptors namely 3,4,5
from each of the three get_unused_fd() calls respectively.

As specified earlier we were able to successfully read using the
third file descriptor.

Please spare time to go through the modifications we made in the
code and help us to sort out this issue.

Thanks in advance,
Sudharsan.

++++++++++++++++++++++++++++++++++++++++++++++++++++++

int do_pipe(int *fd)
{
struct qstr this;

char name[32];

struct dentry *dentry;

struct inode * inode;

//struct file *f1, *f2;
--> struct file *f1, *f2, *f3;

int error;

//int i,j;
--> int i,j,k;

error = -ENFILE;

f1 = get_empty_filp();
if (!f1)
goto no_files;

f2 = get_empty_filp();
if (!f2)
goto close_f1;

--> if (!f3)
--> goto close_f12

inode = get_pipe_inode();

if (!inode)
// goto close_f12;
--> goto close_f123

error = get_unused_fd();

if (error < 0)
goto close_f12_inode;
i = error;

error = get_unused_fd();
if (error < 0)
goto close_f12_inode_i;
j = error;

--> error = get_unused_fd();
--> if (error < 0)
--> goto close_f12_inode_i_j;
--> k=error;

error = -ENOMEM;

sprintf(name, "[%lu]", inode->i_ino);
this.name = name;
this.len = strlen(name);
this.hash = inode->i_ino; /* will go */

dentry = d_alloc(pipe_mnt->mnt_sb->s_root, &this);

if (!dentry)
// goto close_f12_inode_i_j;
--> goto close_f12_inode_i_j_k;

dentry->d_op = &pipefs_dentry_operations;

d_add(dentry, inode);

// f1->f_vfsmnt = f2->f_vfsmnt =
mntget(mntget(pipe_mnt));
--> f1->f_vfsmnt = f2->f_vfsmnt = f3->f_vfsmnt =
mntget(mntget(pipe_mnt));
// f1->f_dentry = f2->f_dentry = dget(dentry);
--> f1->f_dentry = f2->f_dentry = f3->f_dentry =
dget(dentry);
/* read file */

f1->f_pos = f2->f_pos = 0;
// f1->f_flags = O_RDONLY;
--> f1->f_flags = O_WRONLY;
//f1->f_op = &read_pipe_fops;
--> f1->f_op = &write_pipe_fops;
f1->f_mode = 1;
f1->f_version = 0;

/* write file */
// f2->f_flags = O_WRONLY;
--> f2->f_flags = O_RDONLY;
// f2->f_op = &write_pipe_fops;
--> f2->f_op = &read_pipe_fops;
f2->f_mode = 2;
f2->f_version = 0;

--> f3->f_flags = O_RDONLY;
--> f3->f_op = &read_pipe_fops;
f3->f_mode = 2;
f3->f_version = 0;

fd_install(i, f1);
fd_install(j, f2);
--> fd_install(k, f3);

fd[0] = i;
fd[1] = j;
--> fd[2] = k;

return 0;

--> close_f12_inode_i_j_k :
--> put_unused_fd(k);

close_f12_inode_i_j:
put_unused_fd(j);

close_f12_inode_i:
put_unused_fd(i);

close_f12_inode:
free_page((unsigned long) PIPE_BASE(*inode));
kfree(inode->i_pipe);
inode->i_pipe = NULL;
iput(inode);

--> close_f123:
--> put_filp(f3);

close_f12:
put_filp(f2);

close_f1:
put_filp(f1);

no_files:
return error;
}

++++++++++++++++++++++++++++++++++++++++++++++++


2003-03-06 22:56:34

by Mikael Pettersson

[permalink] [raw]
Subject: Re: fd_install question ??

sudharsan vijayaraghavan writes:
> // f1->f_vfsmnt = f2->f_vfsmnt =
> mntget(mntget(pipe_mnt));
> --> f1->f_vfsmnt = f2->f_vfsmnt = f3->f_vfsmnt =
> mntget(mntget(pipe_mnt));
> // f1->f_dentry = f2->f_dentry = dget(dentry);
> --> f1->f_dentry = f2->f_dentry = f3->f_dentry =
> dget(dentry);

A unified diff would have been much more readable.

You now have one more reference to ->f_vfsmnt and ->f_dentry, so I
suspect you're missing one mntget() and one dget() above. The usual
impact of a too low ref count is that the object is reassigned while
you're still using it, with memory corruption & oopses as the result.

/Mikael

2003-03-06 23:18:36

by Chris Wright

[permalink] [raw]
Subject: Re: fd_install question ??

* sudharsan vijayaraghavan ([email protected]) wrote:
> f1 = get_empty_filp();
> if (!f1)
> goto no_files;
>
> f2 = get_empty_filp();
> if (!f2)
> goto close_f1;
>
> --> if (!f3)
> --> goto close_f12

you don't get a new filp for f3. f3 is going to remain as garbage.
-chris
--
Linux Security Modules http://lsm.immunix.org http://lsm.bkbits.net