[PATCH]nilfs2: Don't load/check invalid cno

[PATCH]nilfs2: Don't load/check invalid cno

[Zhu Yanhai]

nilfs2: Don't load/check cp block if specified cno is larger than the
largest exist one.
nilfs2 would load invalid cp block, and report random inconsistent error
message under this situation before.

Signed-off-by: Zhu Yanhai <zhu.yanhai-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

---
 fs/nilfs2/cpfile.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/fs/nilfs2/cpfile.c b/fs/nilfs2/cpfile.c
index aec942c..43978a9 100644
--- a/fs/nilfs2/cpfile.c
+++ b/fs/nilfs2/cpfile.c
@@ -814,9 +814,10 @@ int nilfs_cpfile_is_snapshot(struct inode *cpfile, __u64 cno)
 	struct nilfs_checkpoint *cp;
 	void *kaddr;
 	int ret;
-
-	if (cno == 0)
-		return -ENOENT; /* checkpoint number 0 is invalid */
+
+	/* return ENOENT if cno is invalid. */
+	if (cno == 0 || cno >= nilfs_mdt_cno(cpfile))
+		return -ENOENT;
 	down_read(&NILFS_MDT(cpfile)->mi_sem);
 
 	ret = nilfs_cpfile_get_checkpoint_block(cpfile, cno, 0, &bh);
-- 
1.6.2.2

Re: [PATCH]nilfs2: Don't load/check invalid cno

[Zhu Yanhai]

Below inconsistent error messages will be reported without this patch.

[root@zyh-fedora ~]# mount -t nilfs2 /dev/loop0 ./mount1
mount.nilfs2: WARNING! - The NILFS on-disk format may change at any time.
mount.nilfs2: WARNING! - Do not place critical data on a NILFS filesystem.
[root@zyh-fedora ~]# lscp
                 CNO        DATE     TIME  MODE  FLG   NBLKINC       ICNT
                   1  2009-07-26 16:14:26   cp    -         11          3
                   2  2009-07-26 16:16:12   cp    -        637        144
                   3  2009-07-26 16:16:47   cp    -         17        143
                   4  2009-07-26 16:16:52   cp    -         16        142
                   5  2009-07-26 16:16:58   cp    -         11        142
                   6  2009-07-26 16:17:08   cp    -         18        141
[root@zyh-fedora ~]# mount -t nilfs2 -r -o cp=10 /dev/loop0 ./mount2
mount.nilfs2: Error while mounting /dev/loop0 on ./mount2: Invalid argument
[root@zyh-fedora ~]# mount -t nilfs2 -r -o cp=20 /dev/loop0 ./mount2
mount.nilfs2: Error while mounting /dev/loop0 on ./mount2: Invalid argument
[root@zyh-fedora ~]# mount -t nilfs2 -r -o cp=30 /dev/loop0 ./mount2
mount.nilfs2: Error while mounting /dev/loop0 on ./mount2: No such
file or directory
[root@zyh-fedora ~]# mount -t nilfs2 -r -o cp=40 /dev/loop0 ./mount2
mount.nilfs2: Error while mounting /dev/loop0 on ./mount2: No such
file or directory
[root@zyh-fedora ~]# mount -t nilfs2 -r -o cp=25 /dev/loop0 ./mount2
mount.nilfs2: Error while mounting /dev/loop0 on ./mount2: No such
file or directory
[root@zyh-fedora ~]# mount -t nilfs2 -r -o cp=21 /dev/loop0 ./mount2
mount.nilfs2: Error while mounting /dev/loop0 on ./mount2: No such
file or directory




2009/7/27 Zhu Yanhai <zhu.yanhai@gmail.com>:
> nilfs2: Don't load/check cp block if specified cno is larger than the
> largest exist one.
> nilfs2 would load invalid cp block, and report random inconsistent error
> message under this situation before.
>
> Signed-off-by: Zhu Yanhai <zhu.yanhai@gmail.com>
>
> ---
>  fs/nilfs2/cpfile.c |    7 ++++---
>  1 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/fs/nilfs2/cpfile.c b/fs/nilfs2/cpfile.c
> index aec942c..43978a9 100644
> --- a/fs/nilfs2/cpfile.c
> +++ b/fs/nilfs2/cpfile.c
> @@ -814,9 +814,10 @@ int nilfs_cpfile_is_snapshot(struct inode *cpfile, __u64 cno)
>        struct nilfs_checkpoint *cp;
>        void *kaddr;
>        int ret;
> -
> -       if (cno == 0)
> -               return -ENOENT; /* checkpoint number 0 is invalid */
> +
> +       /* return ENOENT if cno is invalid. */
> +       if (cno == 0 || cno >= nilfs_mdt_cno(cpfile))
> +               return -ENOENT;
>        down_read(&NILFS_MDT(cpfile)->mi_sem);
>
>        ret = nilfs_cpfile_get_checkpoint_block(cpfile, cno, 0, &bh);
> --
> 1.6.2.2
>
_______________________________________________
users mailing list
users@nilfs.org
https://www.nilfs.org/mailman/listinfo/users

Re: [PATCH]nilfs2: Don't load/check invalid cno

[Ryusuke Konishi]

Hi!
On Mon, 27 Jul 2009 17:26:58 +0800, Zhu Yanhai wrote:
> Below inconsistent error messages will be reported without this patch.
> 
> [root@zyh-fedora ~]# mount -t nilfs2 /dev/loop0 ./mount1
> mount.nilfs2: WARNING! - The NILFS on-disk format may change at any time.
> mount.nilfs2: WARNING! - Do not place critical data on a NILFS filesystem.
> [root@zyh-fedora ~]# lscp
>                  CNO        DATE     TIME  MODE  FLG   NBLKINC       ICNT
>                    1  2009-07-26 16:14:26   cp    -         11          3
>                    2  2009-07-26 16:16:12   cp    -        637        144
>                    3  2009-07-26 16:16:47   cp    -         17        143
>                    4  2009-07-26 16:16:52   cp    -         16        142
>                    5  2009-07-26 16:16:58   cp    -         11        142
>                    6  2009-07-26 16:17:08   cp    -         18        141
> [root@zyh-fedora ~]# mount -t nilfs2 -r -o cp=10 /dev/loop0 ./mount2
> mount.nilfs2: Error while mounting /dev/loop0 on ./mount2: Invalid argument
> [root@zyh-fedora ~]# mount -t nilfs2 -r -o cp=20 /dev/loop0 ./mount2
> mount.nilfs2: Error while mounting /dev/loop0 on ./mount2: Invalid argument
> [root@zyh-fedora ~]# mount -t nilfs2 -r -o cp=30 /dev/loop0 ./mount2
> mount.nilfs2: Error while mounting /dev/loop0 on ./mount2: No such
> file or directory
> [root@zyh-fedora ~]# mount -t nilfs2 -r -o cp=40 /dev/loop0 ./mount2
> mount.nilfs2: Error while mounting /dev/loop0 on ./mount2: No such
> file or directory
> [root@zyh-fedora ~]# mount -t nilfs2 -r -o cp=25 /dev/loop0 ./mount2
> mount.nilfs2: Error while mounting /dev/loop0 on ./mount2: No such
> file or directory
> [root@zyh-fedora ~]# mount -t nilfs2 -r -o cp=21 /dev/loop0 ./mount2
> mount.nilfs2: Error while mounting /dev/loop0 on ./mount2: No such
> file or directory
 
Thank you for pointing out the inconsistency.

Your patch noticed me that a few problems exist around the
nilfs_cpfile_is_snapshot() function:

 1) For invalid checkpoints it should return an ENOENT error, but this
    check is missing; a test with nilfs_checkpoint_invalid() should
    come before the nilfs_checkpoint_snapshot() test.

 2) If nilfs_cpfile_is_snapshot() returned an ENOENT error,
    nilfs_fill_super() should convert it to -EINVAL, but this is
    missing too.  (the ENOENT code is for internal use)

 3) Snapshots can be mounted concurrently with a writable mount.  So,
    the call site of nilfs_cpfile_is_snapshot() must be protected from
    segment writer.  I mean it should hold read lock for
    nilfs->ns_segctor_sem during the call-out like:

     down_read(&nilfs->ns_segctor_sem);
     err = nilfs_cpfile_is_snapshot(nilfs->ns_cpfile, sbi->s_snapshot_cno);
     up_read(&nilfs->ns_segctor_sem);

    This is needed even to protect the use of nilfs_mdt_cno().


Then, (1) and (2) would fix the inconsistency problem.

But I think your patch is still needed to exclude the next checkpoint
for snapshot mount.

A little bit problematic.. up to four fixes.

I would appreciate it if you could rewrite patches taking the above
into account.

Thanks in advance,
Ryusuke Konishi


> 2009/7/27 Zhu Yanhai <zhu.yanhai-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
> > nilfs2: Don't load/check cp block if specified cno is larger than the
> > largest exist one.
> > nilfs2 would load invalid cp block, and report random inconsistent error
> > message under this situation before.
> >
> > Signed-off-by: Zhu Yanhai <zhu.yanhai-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >
> > ---
> >  fs/nilfs2/cpfile.c |    7 ++++---
> >  1 files changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/fs/nilfs2/cpfile.c b/fs/nilfs2/cpfile.c
> > index aec942c..43978a9 100644
> > --- a/fs/nilfs2/cpfile.c
> > +++ b/fs/nilfs2/cpfile.c
> > @@ -814,9 +814,10 @@ int nilfs_cpfile_is_snapshot(struct inode *cpfile, __u64 cno)
> >        struct nilfs_checkpoint *cp;
> >        void *kaddr;
> >        int ret;
> > -
> > -       if (cno == 0)
> > -               return -ENOENT; /* checkpoint number 0 is invalid */
> > +
> > +       /* return ENOENT if cno is invalid. */
> > +       if (cno == 0 || cno >= nilfs_mdt_cno(cpfile))
> > +               return -ENOENT;
> >        down_read(&NILFS_MDT(cpfile)->mi_sem);
> >
> >        ret = nilfs_cpfile_get_checkpoint_block(cpfile, cno, 0, &bh);
> > --
> > 1.6.2.2
> >

Re: [PATCH]nilfs2: Don't load/check invalid cno

[Zhu Yanhai]

Thank you for your comments, I will revise it and resend later.

Regards,
Zhu Yanhai

2009/7/28 Ryusuke Konishi <ryusuke@osrg.net>:
> Hi!
> On Mon, 27 Jul 2009 17:26:58 +0800, Zhu Yanhai wrote:
>> Below inconsistent error messages will be reported without this patch.
>>
>> [root@zyh-fedora ~]# mount -t nilfs2 /dev/loop0 ./mount1
>> mount.nilfs2: WARNING! - The NILFS on-disk format may change at any time.
>> mount.nilfs2: WARNING! - Do not place critical data on a NILFS filesystem.
>> [root@zyh-fedora ~]# lscp
>>                  CNO        DATE     TIME  MODE  FLG   NBLKINC       ICNT
>>                    1  2009-07-26 16:14:26   cp    -         11          3
>>                    2  2009-07-26 16:16:12   cp    -        637        144
>>                    3  2009-07-26 16:16:47   cp    -         17        143
>>                    4  2009-07-26 16:16:52   cp    -         16        142
>>                    5  2009-07-26 16:16:58   cp    -         11        142
>>                    6  2009-07-26 16:17:08   cp    -         18        141
>> [root@zyh-fedora ~]# mount -t nilfs2 -r -o cp=10 /dev/loop0 ./mount2
>> mount.nilfs2: Error while mounting /dev/loop0 on ./mount2: Invalid argument
>> [root@zyh-fedora ~]# mount -t nilfs2 -r -o cp=20 /dev/loop0 ./mount2
>> mount.nilfs2: Error while mounting /dev/loop0 on ./mount2: Invalid argument
>> [root@zyh-fedora ~]# mount -t nilfs2 -r -o cp=30 /dev/loop0 ./mount2
>> mount.nilfs2: Error while mounting /dev/loop0 on ./mount2: No such
>> file or directory
>> [root@zyh-fedora ~]# mount -t nilfs2 -r -o cp=40 /dev/loop0 ./mount2
>> mount.nilfs2: Error while mounting /dev/loop0 on ./mount2: No such
>> file or directory
>> [root@zyh-fedora ~]# mount -t nilfs2 -r -o cp=25 /dev/loop0 ./mount2
>> mount.nilfs2: Error while mounting /dev/loop0 on ./mount2: No such
>> file or directory
>> [root@zyh-fedora ~]# mount -t nilfs2 -r -o cp=21 /dev/loop0 ./mount2
>> mount.nilfs2: Error while mounting /dev/loop0 on ./mount2: No such
>> file or directory
>
> Thank you for pointing out the inconsistency.
>
> Your patch noticed me that a few problems exist around the
> nilfs_cpfile_is_snapshot() function:
>
>  1) For invalid checkpoints it should return an ENOENT error, but this
>    check is missing; a test with nilfs_checkpoint_invalid() should
>    come before the nilfs_checkpoint_snapshot() test.
>
>  2) If nilfs_cpfile_is_snapshot() returned an ENOENT error,
>    nilfs_fill_super() should convert it to -EINVAL, but this is
>    missing too.  (the ENOENT code is for internal use)
>
>  3) Snapshots can be mounted concurrently with a writable mount.  So,
>    the call site of nilfs_cpfile_is_snapshot() must be protected from
>    segment writer.  I mean it should hold read lock for
>    nilfs->ns_segctor_sem during the call-out like:
>
>     down_read(&nilfs->ns_segctor_sem);
>     err = nilfs_cpfile_is_snapshot(nilfs->ns_cpfile, sbi->s_snapshot_cno);
>     up_read(&nilfs->ns_segctor_sem);
>
>    This is needed even to protect the use of nilfs_mdt_cno().
>
>
> Then, (1) and (2) would fix the inconsistency problem.
>
> But I think your patch is still needed to exclude the next checkpoint
> for snapshot mount.
>
> A little bit problematic.. up to four fixes.
>
> I would appreciate it if you could rewrite patches taking the above
> into account.
>
> Thanks in advance,
> Ryusuke Konishi
>
>
>> 2009/7/27 Zhu Yanhai <zhu.yanhai@gmail.com>:
>> > nilfs2: Don't load/check cp block if specified cno is larger than the
>> > largest exist one.
>> > nilfs2 would load invalid cp block, and report random inconsistent error
>> > message under this situation before.
>> >
>> > Signed-off-by: Zhu Yanhai <zhu.yanhai@gmail.com>
>> >
>> > ---
>> >  fs/nilfs2/cpfile.c |    7 ++++---
>> >  1 files changed, 4 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/fs/nilfs2/cpfile.c b/fs/nilfs2/cpfile.c
>> > index aec942c..43978a9 100644
>> > --- a/fs/nilfs2/cpfile.c
>> > +++ b/fs/nilfs2/cpfile.c
>> > @@ -814,9 +814,10 @@ int nilfs_cpfile_is_snapshot(struct inode *cpfile, __u64 cno)
>> >        struct nilfs_checkpoint *cp;
>> >        void *kaddr;
>> >        int ret;
>> > -
>> > -       if (cno == 0)
>> > -               return -ENOENT; /* checkpoint number 0 is invalid */
>> > +
>> > +       /* return ENOENT if cno is invalid. */
>> > +       if (cno == 0 || cno >= nilfs_mdt_cno(cpfile))
>> > +               return -ENOENT;
>> >        down_read(&NILFS_MDT(cpfile)->mi_sem);
>> >
>> >        ret = nilfs_cpfile_get_checkpoint_block(cpfile, cno, 0, &bh);
>> > --
>> > 1.6.2.2
>> >
>
_______________________________________________
users mailing list
users@nilfs.org
https://www.nilfs.org/mailman/listinfo/users