Skip to contentSkip to conversation

nilfs2: Don't load/check invalid cno

PATCH In review Zhu Yanhai 2 participants 4 messages Jul 27, 2009 – Jul 28, 2009 3 messages not in the archive NILFS2 filesystem

lore
Share & tools
lore

Zhu Yanhai

author
#0 lore raw

PATCH 1/1 nilfs2: Don't load/check invalid cno

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 <[email protected]>
---
 fs/nilfs2/cpfile.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)
fs/nilfs2/cpfile.c +4 -3
index aec942c..43978a9 100644
@@ -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);
Signature · 1 line
1.6.2.2

A message that is not in the archive (sent off-list or not yet received): [email protected]

author
#2 lore raw

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 <[email protected]>:

Quoted 30 lines

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 <[email protected]>

---
 fs/nilfs2/cpfile.c |    7 ++++---

 1 files changed, 4 insertions(+), 3 deletions(-)
fs/nilfs2/cpfile.c +4 -4
index aec942c..43978a9 100644
@@ -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);
--
Quoted 2 lines

1.6.2.2

_______________________________________________
users mailing list
[email protected]
https://www.nilfs.org/mailman/listinfo/users

A message that is not in the archive (sent off-list or not yet received): 977a2be20907270226s7c7037b5v38830e1ba68cefcb-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org

#4 lore raw

Hi!

Quoted 13 lines · 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
Quoted 16 lines

[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

Quoted 33 lines

2009/7/27 Zhu Yanhai <[email protected]>:

Quoted 32 lines

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 <[email protected]>

---
 fs/nilfs2/cpfile.c |    7 ++++---

 1 files changed, 4 insertions(+), 3 deletions(-)
fs/nilfs2/cpfile.c +4 -4
index aec942c..43978a9 100644
@@ -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

A message that is not in the archive (sent off-list or not yet received): [email protected]

author
#6 lore raw

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

Regards,
Zhu Yanhai

2009/7/28 Ryusuke Konishi <[email protected]>:

Quoted 100 lines

Hi!

Quoted 21 lines · 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

Quoted 8 lines

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

Quoted 29 lines

2009/7/27 Zhu Yanhai <[email protected]>:

Quoted 28 lines

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 <[email protected]>

---
 fs/nilfs2/cpfile.c |    7 ++++---

 1 files changed, 4 insertions(+), 3 deletions(-)
fs/nilfs2/cpfile.c +4 -3
index aec942c..43978a9 100644
@@ -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);
Quoted 5 lines
Quoted 4 lines
Quoted 4 lines
fs/nilfs2/cpfile.c +0 -1
        ret = nilfs_cpfile_get_checkpoint_block(cpfile, cno, 0, &bh);
--

1.6.2.2

_______________________________________________
users mailing list
[email protected]
https://www.nilfs.org/mailman/listinfo/users