Re: PATCH: smart symlink loop detection.

Adam D. Bradley (artdodge@cs.bu.edu)
Mon, 20 Apr 1998 16:36:25 -0400 (EDT)


On 20 Apr 1998, H. Peter Anvin wrote:

> By author: "Adam D. Bradley" <artdodge@cs.bu.edu>
>
> > There's just one problem: I need to call inode->i_op->readlink() to
> > get the "value" of a symlink. ext2_readlink (among others I suspect)
> > uses copy_to_user(), which _sometimes_ returns -EFAULT when trying to
> > write the symlink's text to the buffers I'm using. (This happens w/
> > all sorts of buffers: static char[]'s in the fn, kmalloc(PATH_MAX+1,
> > GFP_{KERNEL,USER}), and get_free_page(GFP_{KERNEL,USER}).) My guess
> > is it doesn't like writing to kernel-space buffers. BSD handles this
> > using a flag in their "uio" structure. I'm sure this can be done in
> > Linux, there's just something obvious I'm missing.
>
> Another problem: it's wrong; readlink() and follow_link() are
> *different* operations under Linux. You need to use follow_link(),
> and if the interface of follow_link() doesn't match what you want then
> you have to change the VFS.

I'm not sure I'm clear on what the problem is... readlink() is only
being used to get the _text_ of the symlink's "value", not to actually
follow it. This is what readlink is _supposed_ to do, right? (At
least, that's what is does in ext2 and nfs.) And it's only used when
we _know_ the dentry in hand points to a symlink; otherwise, I use the
follow_link operation as usual. Something like this:

if (IS_SYMLINK(dentry))
inode->i_op->readlink(dentry,buffer,MAX_FILENAME);
namep -= strlen(buffer);
strcpy(namep, buffer, strlen(buffer);
continue; /* re-start the lookup_dentry loop so
it will use name pulled fm symlink */
else
inode->follow_link(base,dentry);

> > Another nit-pick - since there's no way for me to know (w/o using
> > fs-specific code) the length of a symlink's value until I've copied it
> > (return of i_op->readlink), I need to use two buffers. If there were
> > a way to know beforehand, I could dispense with that second buffer (a
> > second page of memory) and do everything in-place. This would,
> > however, probably require a new i_op.
>
> You need that anyway. See above. Also, be *very* aware of the
> potential for race conditions here.

Agreed, having separate operations to get the length and then read the
value would be dangerous. The goal is to remove an extra copy, so
an alternative is a protected "readlink_negative" operation, which is
given a pointer to the _end_ of the buffer and copies the link's value
"backwards" from there.

Adam

--
You crucify all honesty             \\Adam D. Bradley  artdodge@cs.bu.edu
No signs you see do you believe      \\Boston University Computer Science
And all your words just twist and turn\\    Grad Student and Linux Hacker
Reviving just to crash and burn        \\                             <><
--------->   Why can't you listen as love screams everywhere?   <--------

- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.rutgers.edu