Regression caused by commit c83ed4c9dbb3 "ubifs: Abort readdir on error"

Regression caused by commit c83ed4c9dbb3 "ubifs: Abort readdir on error"

[Peter Rosin]

Hi!

Early on in my boot procedure I create an overlay and shuffle
things around a bit so that the original rootfs stays readonly
at the bottom of an overlayed readwrite fs. This way I can just
toss out the overlayed fs and have a clean and reliable way to
do a "factory-reset". Both the read-only root and and the overlay
are ubifs on nand.

Bisection points to commit c83ed4c9dbb3 "ubifs: Abort readdir
on error". Reverting that commit on top of v4.9-rc2 fixes things.

My "shuffle script" is as follows (with an added comment
indicating the first point of failure) and runs as the first
script by init.

If I add a line
	ls -l /mnt || echo "ls -l /mnt failed"
where that comment is right before the failure, I get no failure
indication even with the broken commit, so directory traversal
seems ok.

Cheers,
Peter

#! /bin/bash

test "x$1" == xstart || exit 0

mkdir /tmp/log

/bin/mount -o remount,rw /dev

/bin/mount | /bin/grep -q "^overlay on / type overlay" && exit 0

if test -e /etc/factory-reset; then
  /usr/sbin/ubiformat /dev/mtd7
  rm /etc/factory-reset
fi

/usr/sbin/ubiattach /dev/ubi_ctrl -m 7
/usr/sbin/ubimkvol /dev/ubi1 -N ovl -m

/bin/mkdir -p /mnt/ovl
/bin/mkdir -p /mnt/roroot
/bin/mkdir -p /mnt/merge
/bin/mount -t ubifs /dev/ubi1_0 /mnt/ovl

/bin/mkdir -p /mnt/ovl/ovl
/bin/mkdir -p /mnt/ovl/work
# with c83ed4c9dbb3, the following mount fails with
# mount: mounting overlay on /mnt/merge failed: No such file or directory
/bin/mount -t overlay -o lowerdir=/,upperdir=/mnt/ovl/ovl,workdir=/mnt/ovl/work overlay /mnt/merge

cd /mnt/merge

/sbin/pivot_root . /mnt/merge/mnt/roroot

/bin/mount --rbind /mnt/roroot/proc /proc
/bin/mount --rbind /mnt/roroot/sys /sys
/bin/mount --rbind /mnt/roroot/dev /dev
/bin/mount --rbind /mnt/roroot/tmp /tmp
/bin/mount --rbind /mnt/roroot/run /run
/bin/mount --rbind /mnt/roroot/mnt/ovl /mnt/ovl

kill $PPID
kill -QUIT 1

Re: Regression caused by commit c83ed4c9dbb3 "ubifs: Abort readdir on error"

[Richard Weinberger]

Peter,

On 25.10.2016 21:39, Peter Rosin wrote:
> Hi!
> 
> Early on in my boot procedure I create an overlay and shuffle
> things around a bit so that the original rootfs stays readonly
> at the bottom of an overlayed readwrite fs. This way I can just
> toss out the overlayed fs and have a clean and reliable way to
> do a "factory-reset". Both the read-only root and and the overlay
> are ubifs on nand.
> 
> Bisection points to commit c83ed4c9dbb3 "ubifs: Abort readdir
> on error". Reverting that commit on top of v4.9-rc2 fixes things.
> 
> My "shuffle script" is as follows (with an added comment
> indicating the first point of failure) and runs as the first
> script by init.
> 
> If I add a line
> 	ls -l /mnt || echo "ls -l /mnt failed"
> where that comment is right before the failure, I get no failure
> indication even with the broken commit, so directory traversal
> seems ok.
> 
> Cheers,
> Peter
> 
> #! /bin/bash
> 
> test "x$1" == xstart || exit 0
> 
> mkdir /tmp/log
> 
> /bin/mount -o remount,rw /dev
> 
> /bin/mount | /bin/grep -q "^overlay on / type overlay" && exit 0
> 
> if test -e /etc/factory-reset; then
>   /usr/sbin/ubiformat /dev/mtd7
>   rm /etc/factory-reset
> fi
> 
> /usr/sbin/ubiattach /dev/ubi_ctrl -m 7
> /usr/sbin/ubimkvol /dev/ubi1 -N ovl -m
> 
> /bin/mkdir -p /mnt/ovl
> /bin/mkdir -p /mnt/roroot
> /bin/mkdir -p /mnt/merge
> /bin/mount -t ubifs /dev/ubi1_0 /mnt/ovl
> 
> /bin/mkdir -p /mnt/ovl/ovl
> /bin/mkdir -p /mnt/ovl/work
> # with c83ed4c9dbb3, the following mount fails with
> # mount: mounting overlay on /mnt/merge failed: No such file or directory
> /bin/mount -t overlay -o lowerdir=/,upperdir=/mnt/ovl/ovl,workdir=/mnt/ovl/work overlay /mnt/merge

Uhhhh! Thanks for catching this.
I think I know what is going on, the fix exposes now internal UBIFS return codes to VFS.
Let me prepare a fix.

Thanks,
//richard

Re: Regression caused by commit c83ed4c9dbb3 "ubifs: Abort readdir on error"

[Richard Weinberger]

[-- Attachment #1: Type: text/plain, Size: 563 bytes --]

Peter,

On 25.10.2016 22:09, Richard Weinberger wrote:
>> /bin/mkdir -p /mnt/ovl/ovl
>> /bin/mkdir -p /mnt/ovl/work
>> # with c83ed4c9dbb3, the following mount fails with
>> # mount: mounting overlay on /mnt/merge failed: No such file or directory
>> /bin/mount -t overlay -o lowerdir=/,upperdir=/mnt/ovl/ovl,workdir=/mnt/ovl/work overlay /mnt/merge
> 
> Uhhhh! Thanks for catching this.
> I think I know what is going on, the fix exposes now internal UBIFS return codes to VFS.
> Let me prepare a fix.

Does the attached patch cure the issue?

Thanks,
//richard

[-- Attachment #2: ubifs_readdir_fix_fix.diff --]
[-- Type: text/x-patch, Size: 596 bytes --]

diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
index bd4a5e8ce441..ca16c5d7bab1 100644
--- a/fs/ubifs/dir.c
+++ b/fs/ubifs/dir.c
@@ -543,6 +543,14 @@ static int ubifs_readdir(struct file *file, struct dir_context *ctx)
 
 	if (err != -ENOENT)
 		ubifs_err(c, "cannot find next direntry, error %d", err);
+	else
+		/*
+		 * -ENOENT is a non-fatal error in this context, the TNC uses
+		 * it to indicate that the cursor moved past the current directory
+		 * and readdir() has to stop.
+		 */
+		err = 0;
+
 
 	/* 2 is a special value indicating that there are no more direntries */
 	ctx->pos = 2;

Re: Regression caused by commit c83ed4c9dbb3 "ubifs: Abort readdir on error"

[Peter Rosin]

Hi Richard,

On 2016-10-25 22:50, Richard Weinberger wrote:
> Peter,
> 
> On 25.10.2016 22:09, Richard Weinberger wrote:
>>> /bin/mkdir -p /mnt/ovl/ovl
>>> /bin/mkdir -p /mnt/ovl/work
>>> # with c83ed4c9dbb3, the following mount fails with
>>> # mount: mounting overlay on /mnt/merge failed: No such file or directory
>>> /bin/mount -t overlay -o lowerdir=/,upperdir=/mnt/ovl/ovl,workdir=/mnt/ovl/work overlay /mnt/merge
>>
>> Uhhhh! Thanks for catching this.
>> I think I know what is going on, the fix exposes now internal UBIFS return codes to VFS.
>> Let me prepare a fix.
> 
> Does the attached patch cure the issue?

Yup, back to normal AFAICT. Thanks for the speedy fix!

Reported-by: Peter Rosin <peda@axentia.se>
Tested-by: Peter Rosin <peda@axentia.se>

Cheers,
Peter