Re: [Yaffs] Possible soft deletes issue

On Wednesday 12 May 2010 05:17:42 Ross Younger wrote:
> Hi all,
>
> I've recently synched up with the latest from the git repo and am trying
> out multiple code paths.
>
> I'm seeing an issue with file deletion in one particular case:
> * small page (synthetic) flash, so yaffs1 mode
> * soft deletes are enabled
> * my driver (eCos) is not using the cached inode facility, i.e. myInode is
> NULL for all objects
>
> The issue seems to arise as yaffs_DeleteFile calls
> yaffs_UnlinkFileIfNeeded. In UnlinkFileIfNeeded, immediateDeletion becomes
> 1 and (amongst other things) in->deleted is set. However, DeleteFile then
> goes on to look only at its own on-stack `deleted' variable in determining
> whether or not the operation has succeeded - which has not been updated in
> this case, so it returns what appears to be a spurious failure.
>
> Patch attached for your consideration; it passes my test cases which were
> failing, and I'm running a soak test as I type this.

Ross

I would suggest you modify the eCOS binding to use dummy myInode values
consistently with the way Linux and yaffs direct do this since this is what
gets the main testing.

git blame helped remind me why the stacked deleted variable was added.

The problem is this: under certain conditions the object pointed to by in can
be freed any you'd end up dereferencing a freed pointer.

That's probably going to work if you're using the yaffs object cache layer
because freeing does not really free the memory.

Instead I propose something a bit different. This just moves reading the value
in case it is changed by yaffs_UnlinkFileIfNeeded.

Please say whether this works in your soak test.

Thanks

Charles

diff --git a/yaffs_guts.c b/yaffs_guts.c
index d55aa95..9509dd4 100644
--- a/yaffs_guts.c
+++ b/yaffs_guts.c
@@ -5624,7 +5624,7 @@ static int yaffs_UnlinkFileIfNeeded(yaffs_Object *in)
 int yaffs_DeleteFile(yaffs_Object *in)
 {
        int retVal = YAFFS_OK;
-       int deleted = in->deleted;
+       int deleted; /* Need to cache value on stack if in is freed */
        yaffs_Device *dev = in->myDev;

        if (dev->param.disableSoftDelete || dev->param.isYaffs2)
@@ -5637,6 +5637,8 @@ int yaffs_DeleteFile(yaffs_Object *in)
                if (!in->unlinked)
                        retVal = yaffs_UnlinkFileIfNeeded(in);

+               deleted = in->deleted;
+
                if (retVal == YAFFS_OK && in->unlinked && !in->deleted) {
                        in->deleted = 1;
                        deleted = 1;