Bug#190732: [PATCH] hurd/libdiskfs/dir-renamed.c

[Ognyan Kulev]

On Wed, Jul 16, 2003 at 05:59:20AM -0500, Marcus Brinkmann wrote:
> On Wed, Jul 16, 2003 at 01:26:20AM +0300, Ognyan Kulev wrote:
> > On Tue, Jul 15, 2003 at 05:43:24AM -0500, Marcus Brinkmann wrote:
> > > 1. What errors do you still expect from the second time diskfs_lookup is
> > > invoked with REMOVE?  Is there any error that is allowed at that time?
> > > If there is, the change is correct.
> > 
> > Before the second diskfs_lookup, mutex_unlock is called.  This leaves a 
> > little time when someone could remove the node.
> 
> The node is unlocked, but the directory is not.  You can not unlink a
> file while the directory is locked.  There might be other errors to be
> concerned about?

OK, I think I find one case :-)  Between mutex_unlock and 
diskfs_lookup, someone can change the owner of the node via 
file_chown.  If the directory that contains the node is with sticky bit 
set, the call to diskfs_checkdirmod inside diskfs_lookup will fail.

> > > 2. Is it possible, and according to diskfs.h it should be, to just
> > > call dir_lookup with REMOVE only once, at the point you introduced it
> > > in your patch, and store the dirstat until it is needed for the actual
> > > disks_dirremove call.  This saves one lookup call.  Can you try such a
> > > change and look into the code if it is feasible?
> > 
> > While trying to do that now, I recalled why this cannot be done.  If
> > diskfs_lookup is called only once in the beginning, the assertion in 
> > ext2fs/dir.c:716 is triggered with the following commands:
> > 
> > $ mkdir x
> > $ mv x y
> > $ mv y x
> > 
> > It seems that dirstat of ext2fs can break when there are other 
> > activities in the directory.  It's possible that ext2fs is wrong, but I 
> > didn't look further.
> 
> Yeah, this seems to be in particular the case if the source and target
> directory are the same?  I don't have time to really crunch all that
> code until I have figured out what is safe and what is supposed to be
> safe.  The easy fix for now, and likely the thing that is really
> required, is dropping the dirstat as you did in your patch.

Imagine what happens in directory layout when the second mv is executed.  
We have one empty entry in the directory (this was x before it becomes 
y), followed by y.  So preventry (ext2fs.c/dir.c:82) points to the empty 
slot.  But before the file is actually removed, we do diskfs_direnter 
and so preventry is no longer valid, and the assertion failed.

Reading diskfs.h:313, there can be no diskfs_lookup between 
diskfs_lookup and diskfs_{drop_dirstat,dir{enter,rewrite,remove}} in the 
same directory, so the ext2fs server is OK.

I wonder if we could replace diskfs_direnter with diskfs_dirrewrite for 
the case when the directory is the same.  Probably I'll post another 
patch soon.

Regards
-- 
Ognyan Kulev <ogi@{fmi.uni-sofia.bg,fsa-bg.org}>
7D9F 66E6 68B7 A62B 0FCF  EB04 80BF 3A8C A252 9782