Re: misuse of pathnames in rump (and portalfs?)
On Wed, Nov 24, 2010 at 08:30:18PM +0200, Antti Kantee wrote:
> > I think it makes more sense for doregister to check for at least one
> > leading '/' and remove the leading slashes before storing the key.
> > Then the key will match the name passed by lookup; otherwise the
> > leading slash won't be there and it won't match. (What I suggested
> > last night is broken because it doesn't do this.)
>
> Ah, yea, the leading slashes will be stripped for lookup, so we can't
> get an exact match for those anyway.
>
> So, let's define it as "string beginning with /, leading /'s collapsed
> to 1".
Ok. See below (replaces the patch upthread):
> > All users I can find pass an absolute path.
>
> ok, good
diff -r 66985053a079 sys/rump/librump/rumpvfs/rumpfs.c
--- a/sys/rump/librump/rumpvfs/rumpfs.c Wed Nov 24 01:34:10 2010 -0500
+++ b/sys/rump/librump/rumpvfs/rumpfs.c Wed Nov 24 15:41:26 2010 -0500
@@ -324,6 +324,13 @@ doregister(const char *key, const char *
devminor_t dmin = -1;
int hft, error;
+ if (key[0] != '/') {
+ return EINVAL;
+ }
+ while (key[0] == '/') {
+ key++;
+ }
+
if (rumpuser_getfileinfo(hostpath, &fsize, &hft, &error))
return error;
@@ -396,7 +403,7 @@ doregister(const char *key, const char *
if (ftype == RUMP_ETFS_BLK) {
format_bytes(buf, sizeof(buf), size);
- aprint_verbose("%s: hostpath %s (%s)\n", key, hostpath, buf);
+ aprint_verbose("/%s: hostpath %s (%s)\n", key, hostpath, buf);
}
return 0;
@@ -641,13 +648,15 @@ rump_vop_lookup(void *v)
if (dvp == rootvnode && cnp->cn_nameiop == LOOKUP) {
bool found;
mutex_enter(&etfs_lock);
- found = etfs_find(cnp->cn_pnbuf, &et, false);
+ found = etfs_find(cnp->cn_nameptr, &et, false);
mutex_exit(&etfs_lock);
if (found) {
- char *offset;
+ const char *offset;
- offset = strstr(cnp->cn_pnbuf, et->et_key);
+ /* pointless as et_key is always the whole string */
+ /*offset = strstr(cnp->cn_nameptr, et->et_key);*/
+ offset = cnp->cn_nameptr;
KASSERT(offset);
rn = et->et_rn;
--
David A. Holland
dholl...@netbsd.org
Re: misuse of pathnames in rump (and portalfs?)
On Wed, Nov 24, 2010 at 01:26:04PM -0500, der Mouse wrote: > > Right. But if you want a guaranteed absolute path you should be able > > to do it by calling getcwd first. > > Only if you accept breakage if the current directory no longer has any > name. Well, if you can't call getcwd, then it won't work... (I was not suggesting that the getcwd call be wedged inside namei) > Of course, if you consider that acceptable, then fine. I don't, not > for something as central as namei (though this looks as though you may > be talking about only certain filesystems, in which case it may be > acceptable). We'll probably end up doing it on every exec, since there's currently no way to tell from the ELF headers whether $ORIGIN needs to be set (this should be considered a bug) but failure is ok for that. In compat_svr4 I doubt anyone cares much if it fails in corner cases. -- David A. Holland dholl...@netbsd.org
Re: misuse of pathnames in rump (and portalfs?)
On Wed Nov 24 2010 at 18:12:00 +, David Holland wrote: > > As for etfs, as you might be able to see from the code, it's only used > > for root vnode lookups. I cannot think of a reason why we cannot define > > the key to start with exactly one leading '/'. Some in-tree users may > > not follow that rule now, but they should be quite trivial to locate > > with grep. That should make it work properly with your finally-nonbroken > > namei and also take care of all symlink/.. concerns you might have. > > I think it makes more sense for doregister to check for at least one > leading '/' and remove the leading slashes before storing the key. > Then the key will match the name passed by lookup; otherwise the > leading slash won't be there and it won't match. (What I suggested > last night is broken because it doesn't do this.) Ah, yea, the leading slashes will be stripped for lookup, so we can't get an exact match for those anyway. So, let's define it as "string beginning with /, leading /'s collapsed to 1". > All users I can find pass an absolute path. ok, good
Re: misuse of pathnames in rump (and portalfs?)
> Right. But if you want a guaranteed absolute path you should be able > to do it by calling getcwd first. Only if you accept breakage if the current directory no longer has any name. Of course, if you consider that acceptable, then fine. I don't, not for something as central as namei (though this looks as though you may be talking about only certain filesystems, in which case it may be acceptable). /~\ The ASCII Mouse \ / Ribbon Campaign X Against HTMLmo...@rodents-montreal.org / \ Email! 7D C8 61 52 5D E7 2D 39 4E F1 31 3E E8 B3 27 4B
Re: misuse of pathnames in rump (and portalfs?)
On Wed, Nov 24, 2010 at 06:19:50PM +, David Laight wrote: > > First, background: VOP_LOOKUP is the per-fs vnode operation that takes > > a directory and a name and returns the vnode associated with that > > name. This currently has a horrific interface and the chief goal of > > the namei cleanup is to rectify this so all it needs is a vnode and a > > string. > > Presuably also some flags - NOFOLLOW etc no, it shouldn't need any flags (particularly not NOFOLLOW...) > One thing I have wondered (which might be related to what rump > expects) is whether namei could perform 'realpath' as part of its > processing - at least optionally? Yes, it already is supposed to but doesn't quite. There's a compat_svr4 syscall that needs to do this, plus we want this behavior in exec for the ELF $ORIGIN thing. > At least as far as following symlinks and following "/../", the final > path might still be relative (and thus start with ../). Right. But if you want a guaranteed absolute path you should be able to do it by calling getcwd first. This is not that hard though -- the output should be stored into a separate buffer, and if one preseeds this buffer with the getcwd result it should more or less automatically DTRT. -- David A. Holland dholl...@netbsd.org
Re: misuse of pathnames in rump (and portalfs?)
On Tue, Nov 23, 2010 at 11:13:02PM +, David Holland wrote: > I have run into a problem fixing namei. > > First, background: VOP_LOOKUP is the per-fs vnode operation that takes > a directory and a name and returns the vnode associated with that > name. This currently has a horrific interface and the chief goal of > the namei cleanup is to rectify this so all it needs is a vnode and a > string. Presuably also some flags - NOFOLLOW etc One thing I have wondered (which might be related to what rump expects) is whether namei could perform 'realpath' as part of its processing - at least optionally? At least as far as following symlinks and following "/../", the final path might still be relative (and thus start with ../). David -- David Laight: da...@l8s.co.uk
Re: misuse of pathnames in rump (and portalfs?)
On Wed, Nov 24, 2010 at 07:32:42PM +0200, Antti Kantee wrote: > > Furthermore, it is just plain gross for the behavior of VOP_LOOKUP in > > some directory to depend on how one got to that directory. As a matter > > of design, the working path should not be available to VOP_LOOKUP and > > VOP_LOOKUP should not attempt to make use of it. > > > > When I asked pooka for clarification, I got back an assertion that > > portalfs depends on this behavior so I should rethink the namei design > > to support it. However, as far as I can tell, this is not true: there > > is only one unexpected/problematic use of the pathname buffer in > > question anywhere in the system, in rumpfs.c. Furthermore, even if it > > were true, I think it would be highly undesirable. > > You wrote that the whole path will no longer be available. As you > say yourself, it doesn't make sense for a file system to care about > the previous components, so don't be shocked that I took this to > mean the whole remaining path. If the whole remaining path is > available, portalfs should be fine. I don't remember exactly what I wrote, but I'm pretty sure I explicitly said it was using the whole namei work buffer. Perhaps it could have been clearer. I don't see that exposing the whole remaining path is desirable either, but that's a different argument. > As for etfs, as you might be able to see from the code, it's only used > for root vnode lookups. I cannot think of a reason why we cannot define > the key to start with exactly one leading '/'. Some in-tree users may > not follow that rule now, but they should be quite trivial to locate > with grep. That should make it work properly with your finally-nonbroken > namei and also take care of all symlink/.. concerns you might have. I think it makes more sense for doregister to check for at least one leading '/' and remove the leading slashes before storing the key. Then the key will match the name passed by lookup; otherwise the leading slash won't be there and it won't match. (What I suggested last night is broken because it doesn't do this.) All users I can find pass an absolute path. -- David A. Holland dholl...@netbsd.org
Re: misuse of pathnames in rump (and portalfs?)
Hi, On Tue Nov 23 2010 at 23:13:02 +, David Holland wrote: > Furthermore, it is just plain gross for the behavior of VOP_LOOKUP in > some directory to depend on how one got to that directory. As a matter > of design, the working path should not be available to VOP_LOOKUP and > VOP_LOOKUP should not attempt to make use of it. > > When I asked pooka for clarification, I got back an assertion that > portalfs depends on this behavior so I should rethink the namei design > to support it. However, as far as I can tell, this is not true: there > is only one unexpected/problematic use of the pathname buffer in > question anywhere in the system, in rumpfs.c. Furthermore, even if it > were true, I think it would be highly undesirable. You wrote that the whole path will no longer be available. As you say yourself, it doesn't make sense for a file system to care about the previous components, so don't be shocked that I took this to mean the whole remaining path. If the whole remaining path is available, portalfs should be fine. As for etfs, as you might be able to see from the code, it's only used for root vnode lookups. I cannot think of a reason why we cannot define the key to start with exactly one leading '/'. Some in-tree users may not follow that rule now, but they should be quite trivial to locate with grep. That should make it work properly with your finally-nonbroken namei and also take care of all symlink/.. concerns you might have. thanks, antti
Re: misuse of pathnames in rump (and portalfs?)
>>> (Note that this is free -- it would require splicing a getcwd into >>> every namei call.) >> _Not_ free, I assume you mean? > er right, silly editor... :-/ :-รพ I've had that happen to me often enough. > Anyway, it looks as if it's not needed. Just as well. It has (since) occurred to me that it won't even work, unless you're willing to accept a regression in that it will totally break namei when the current directory no longer has any name. /~\ The ASCII Mouse \ / Ribbon Campaign X Against HTMLmo...@rodents-montreal.org / \ Email! 7D C8 61 52 5D E7 2D 39 4E F1 31 3E E8 B3 27 4B
Re: misuse of pathnames in rump (and portalfs?)
On Wed, Nov 24, 2010 at 08:26:42AM -0500, der Mouse wrote: > > (2) does anyone think that a correct namei design should provide a > > correct canonicalized full pathname for its inspection during lookup? > > Not me. > > > (Note that this is free -- it would require splicing a getcwd into > > every namei call.) > > _Not_ free, I assume you mean? er right, silly editor... :-/ Anyway, it looks as if it's not needed. -- David A. Holland dholl...@netbsd.org
Re: misuse of pathnames in rump (and portalfs?)
> However, I discovered today that rumpfs's VOP_LOOKUP implementation > relies on being able to access not just the name to be looked up, but > also the rest of the pathname namei is working on, specifically > including the parts that have already been translated. "Eww." The rest of the path to the right, that makes some sense (though I'd prefer the filesystem handle it using vnodes for the intermediate directories). The rest of the path to the left, that makes no sense. Not to me. > When I asked pooka for clarification, I got back an assertion that > portalfs depends on this behavior so I should rethink the namei > design to support it. If so, I believe portalfs is critically broken and should be pulled until it's fixed. > (1) does anyone think that a correct namei design should provide the > namei pathname scratch space to the FS for its inspection during > lookup? Not me. > (2) does anyone think that a correct namei design should provide a > correct canonicalized full pathname for its inspection during lookup? Not me. > (Note that this is free -- it would require splicing a getcwd into > every namei call.) _Not_ free, I assume you mean? > (3) Does anyone object if, as a way forward, I add an extra argument > const char *partialpath to VOP_LOOKUP to provide the string that rump > wants, until rump is fixed, and then revert it? Well...I don't like it; such "temporary" hacks have a way of becoming rather less temporary than they should. Personally I think the right fix is to fix the interface and, if this breaks rump, and let it stay broken until someone fixes it to not abuse the interface. But I'm hardly the arbiter of such things. > (4) vermilion. Dead jackal brown. /~\ The ASCII Mouse \ / Ribbon Campaign X Against HTMLmo...@rodents-montreal.org / \ Email! 7D C8 61 52 5D E7 2D 39 4E F1 31 3E E8 B3 27 4B
Re: misuse of pathnames in rump (and portalfs?)
On Nov,Wednesday 24 2010, at 5:35 AM, David Holland wrote: > On Tue, Nov 23, 2010 at 11:13:02PM +, David Holland wrote: >> However, I discovered today that rumpfs's VOP_LOOKUP implementation >> relies on being able to access not just the name to be looked up, but >> also the rest of the pathname namei is working on, specifically >> including the parts that have already been translated. > > Ok, on further inspection it appears that this is overly pessimistic. > It looks, rather, as if rumpfs (specifically the etfs logic) is using > the full namei work buffer and hoping that no such parts actually > appear in it, because if they do it'll fail. > > So I think the following change will resolve the problem; can someone > who knows how this is supposed to work check it? (If it's ok, there's > no need to tamper with VOP_LOOKUP.) You can change your change with running whole regression test suite on rump builded with your patch. Regards Adam.
Re: misuse of pathnames in rump (and portalfs?)
On Tue, Nov 23, 2010 at 11:13:02PM +, David Holland wrote:
> However, I discovered today that rumpfs's VOP_LOOKUP implementation
> relies on being able to access not just the name to be looked up, but
> also the rest of the pathname namei is working on, specifically
> including the parts that have already been translated.
Ok, on further inspection it appears that this is overly pessimistic.
It looks, rather, as if rumpfs (specifically the etfs logic) is using
the full namei work buffer and hoping that no such parts actually
appear in it, because if they do it'll fail.
So I think the following change will resolve the problem; can someone
who knows how this is supposed to work check it? (If it's ok, there's
no need to tamper with VOP_LOOKUP.)
Index: rumpfs.c
===
RCS file: /cvsroot/src/sys/rump/librump/rumpvfs/rumpfs.c,v
retrieving revision 1.74
diff -u -p -r1.74 rumpfs.c
--- rumpfs.c22 Nov 2010 15:15:35 - 1.74
+++ rumpfs.c24 Nov 2010 04:31:07 -
@@ -291,10 +291,9 @@ hft_to_vtype(int hft)
}
static bool
-etfs_find(const char *key, struct etfs **etp, bool forceprefix)
+etfs_find(const char *key, size_t keylen, struct etfs **etp, bool forceprefix)
{
struct etfs *et;
- size_t keylen = strlen(key);
KASSERT(mutex_owned(&etfs_lock));
@@ -381,7 +380,7 @@ doregister(const char *key, const char *
rn->rn_flags |= RUMPNODE_DIR_ETSUBS;
mutex_enter(&etfs_lock);
- if (etfs_find(key, NULL, REGDIR(ftype))) {
+ if (etfs_find(key, strlen(key), NULL, REGDIR(ftype))) {
mutex_exit(&etfs_lock);
if (et->et_blkmin != -1)
rumpblk_deregister(hostpath);
@@ -641,13 +640,15 @@ rump_vop_lookup(void *v)
if (dvp == rootvnode && cnp->cn_nameiop == LOOKUP) {
bool found;
mutex_enter(&etfs_lock);
- found = etfs_find(cnp->cn_pnbuf, &et, false);
+ found = etfs_find(cnp->cn_nameptr, cnp->cn_namelen, &et, false);
mutex_exit(&etfs_lock);
if (found) {
- char *offset;
+ const char *offset;
- offset = strstr(cnp->cn_pnbuf, et->et_key);
+ /* pointless as et_key is always the whole string */
+ /*offset = strstr(cnp->cn_nameptr, et->et_key);*/
+ offset = cnp->cn_nameptr;
KASSERT(offset);
rn = et->et_rn;
--
David A. Holland
dholl...@netbsd.org
