The mentioned functions do not pay attention to the error codes returned
by the functions updateSuper(), lmLogInit() and lmLogShutdown(). It brings to
system crash later when writing to log.
The patch adds corresponding code to check and return the error codes
and to print correct error messages in case of errors.
Besides that the lmLogShutdown() function must not be called when 'nointegrity' mount option is provided.
It leads to kernel OOPS.
Found by Linux File System Verification project (linuxtesting.org).
Signed-off-by: Vahram Martirosyan <[email protected]>
Reviewed-by: Gu Zheng <[email protected]>
---
fs/jfs/super.c | 29 ++++++++++++++++++++++-------
1 file changed, 22 insertions(+), 7 deletions(-)
diff --git a/fs/jfs/super.c b/fs/jfs/super.c
index 2003e83..a3d424d 100644
--- a/fs/jfs/super.c
+++ b/fs/jfs/super.c
@@ -611,11 +611,20 @@ static int jfs_freeze(struct super_block *sb)
{
struct jfs_sb_info *sbi = JFS_SBI(sb);
struct jfs_log *log = sbi->log;
+ int rc = 0;
if (!(sb->s_flags & MS_RDONLY)) {
txQuiesce(sb);
- lmLogShutdown(log);
- updateSuper(sb, FM_CLEAN);
+ rc = lmLogShutdown(log);
+ if (rc != 0) {
+ jfs_err("lmLogShutdown failed with return code %d", rc);
+ return rc;
+ }
+ rc = updateSuper(sb, FM_CLEAN);
+ if (rc != 0) {
+ jfs_err("updateSuper failed with return code %d", rc);
+ return rc;
+ }
}
return 0;
}
@@ -627,11 +636,17 @@ static int jfs_unfreeze(struct super_block *sb)
int rc = 0;
if (!(sb->s_flags & MS_RDONLY)) {
- updateSuper(sb, FM_MOUNT);
- if ((rc = lmLogInit(log)))
- jfs_err("jfs_unlock failed with return code %d", rc);
- else
- txResume(sb);
+ rc = updateSuper(sb, FM_MOUNT);
+ if (rc != 0) {
+ jfs_err("updateSuper failed with return code %d", rc);
+ return rc;
+ }
+ rc = lmLogInit(log);
+ if (rc != 0) {
+ jfs_err("lmLogInit failed with return code %d", rc);
+ return rc;
+ }
+ txResume(sb);
}
return 0;
}
--
1.8.2.3
In function jfs_freeze() the log is shut down through lmLogShutdown() call.
When the "nointegrity" mount option is enabled, the log is actually not
initialized. As a result the freeze operation in that case brings to a
kernel OOPS.
The solution is to check if the "nointegrity" option is enabled and if it is not
then shut the log down.
May be this is not the best solution, but at least it fixes the OOPS.
Found by Linux File System Verification project (linuxtesting.org)
Signed-off-by: Vahram Martirosyan <[email protected]>
---
fs/jfs/super.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/fs/jfs/super.c b/fs/jfs/super.c
index a3d424d..9788970 100644
--- a/fs/jfs/super.c
+++ b/fs/jfs/super.c
@@ -615,10 +615,12 @@ static int jfs_freeze(struct super_block *sb)
if (!(sb->s_flags & MS_RDONLY)) {
txQuiesce(sb);
- rc = lmLogShutdown(log);
- if (rc != 0) {
- jfs_err("lmLogShutdown failed with return code %d", rc);
- return rc;
+ if (!log->no_integrity) {
+ rc = lmLogShutdown(log);
+ if (rc != 0) {
+ jfs_err("lmLogShutdown failed with return code %d", rc);
+ return rc;
+ }
}
rc = updateSuper(sb, FM_CLEAN);
if (rc != 0) {
--
1.8.2.3
Hi Vahram,
I saw the same issue in the bugzilla:
https://bugzilla.kernel.org/show_bug.cgi?id=53331,
and I sent out a patch this issue, but I've get any feedback.
In fact, I think it's the right way to fix this issue,
can you help to test it?
Thanks,
Gu
On 05/24/2013 04:57 PM, Vahram Martirosyan wrote:
> In function jfs_freeze() the log is shut down through lmLogShutdown() call.
> When the "nointegrity" mount option is enabled, the log is actually not
> initialized. As a result the freeze operation in that case brings to a
> kernel OOPS.
>
> The solution is to check if the "nointegrity" option is enabled and if it is not
> then shut the log down.
>
> May be this is not the best solution, but at least it fixes the OOPS.
>
> Found by Linux File System Verification project (linuxtesting.org)
>
> Signed-off-by: Vahram Martirosyan <[email protected]>
> ---
> fs/jfs/super.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/fs/jfs/super.c b/fs/jfs/super.c
> index a3d424d..9788970 100644
> --- a/fs/jfs/super.c
> +++ b/fs/jfs/super.c
> @@ -615,10 +615,12 @@ static int jfs_freeze(struct super_block *sb)
>
> if (!(sb->s_flags & MS_RDONLY)) {
> txQuiesce(sb);
> - rc = lmLogShutdown(log);
> - if (rc != 0) {
> - jfs_err("lmLogShutdown failed with return code %d", rc);
> - return rc;
> + if (!log->no_integrity) {
> + rc = lmLogShutdown(log);
> + if (rc != 0) {
> + jfs_err("lmLogShutdown failed with return code %d", rc);
> + return rc;
> + }
> }
> rc = updateSuper(sb, FM_CLEAN);
> if (rc != 0) {
On 05/24/2013 03:57 AM, Vahram Martirosyan wrote:
> The mentioned functions do not pay attention to the error codes returned
> by the functions updateSuper(), lmLogInit() and lmLogShutdown(). It brings to
> system crash later when writing to log.
>
> The patch adds corresponding code to check and return the error codes
> and to print correct error messages in case of errors.
>
> Besides that the lmLogShutdown() function must not be called when 'nointegrity' mount option is provided.
> It leads to kernel OOPS.
>
> Found by Linux File System Verification project (linuxtesting.org).
>
> Signed-off-by: Vahram Martirosyan <[email protected]>
>
> Reviewed-by: Gu Zheng <[email protected]>
> ---
> fs/jfs/super.c | 29 ++++++++++++++++++++++-------
> 1 file changed, 22 insertions(+), 7 deletions(-)
>
> diff --git a/fs/jfs/super.c b/fs/jfs/super.c
> index 2003e83..a3d424d 100644
> --- a/fs/jfs/super.c
> +++ b/fs/jfs/super.c
> @@ -611,11 +611,20 @@ static int jfs_freeze(struct super_block *sb)
> {
> struct jfs_sb_info *sbi = JFS_SBI(sb);
> struct jfs_log *log = sbi->log;
> + int rc = 0;
>
> if (!(sb->s_flags & MS_RDONLY)) {
> txQuiesce(sb);
> - lmLogShutdown(log);
> - updateSuper(sb, FM_CLEAN);
> + rc = lmLogShutdown(log);
> + if (rc != 0) {
> + jfs_err("lmLogShutdown failed with return code %d", rc);
> + return rc;
Hmm. I'm not sure about the nicest way to recover here. txQuiesce() has
been called. This will leave the filesystem effectively frozen, but not
marked as frozen. Maybe better to call jfs_error() and maybe txResume()
in an attempt to unblock any blocked threads.
> + }
> + rc = updateSuper(sb, FM_CLEAN);
> + if (rc != 0) {
> + jfs_err("updateSuper failed with return code %d", rc);
> + return rc;
same here, except we know that lmLogShutdown() has already succeeded.
Maybe we're better off ignoring this return code, since the freezing
succeeded. Failing to mark the filesystem clean is hardly damaging. It
just means that fsck will work harder next time, which is probably a
good idea. This is an unlikely error if everything else has succeeded.
> + }
> }
> return 0;
> }
> @@ -627,11 +636,17 @@ static int jfs_unfreeze(struct super_block *sb)
> int rc = 0;
>
> if (!(sb->s_flags & MS_RDONLY)) {
> - updateSuper(sb, FM_MOUNT);
> - if ((rc = lmLogInit(log)))
> - jfs_err("jfs_unlock failed with return code %d", rc);
> - else
> - txResume(sb);
> + rc = updateSuper(sb, FM_MOUNT);
> + if (rc != 0) {
> + jfs_err("updateSuper failed with return code %d", rc);
> + return rc;
I think I like calling jfs_error() here and letting the system panic,
the filesystem to be marked as read-only (default), or whatever action
is desired.
> + }
> + rc = lmLogInit(log);
> + if (rc != 0) {
> + jfs_err("lmLogInit failed with return code %d", rc);
> + return rc;
same here
> + }
> + txResume(sb);
> }
> return 0;
> }
I'm going to tweak your patch a bit and see what kind of improvement I
can make.
Thanks,
Shaggy
On 05/24/2013 04:25 AM, Gu Zheng wrote:
> Hi Vahram,
> I saw the same issue in the bugzilla:
> https://bugzilla.kernel.org/show_bug.cgi?id=53331,
> and I sent out a patch this issue, but I've get any feedback.
Sorry I missed that bug. I just realized that bugzilla.kernel.org has
been sending my email to my old IBM address. I've got some catching up
to do.
> In fact, I think it's the right way to fix this issue,
> can you help to test it?
I might go with your patch for now, as it is an improvement, but I don't
really like the way that lmLogShutdown is skipped altogether during a
"nointegrity" unmount. If we skip it during a "nointegrity" freeze, we
should also skip lmLogInit on "nointegrity" unfreeze. I'd like to keep
the nointegrity logic in jfs_logmgr.c, but it would be easiest to fix in
the freeze and unfreeze functions. A code cleanup may be better than the
easy fix. I'll have to work on that.
It also looks like freeze/unfreeze doesn't take into account that two or
more mounted file systems can share the same journal. I don't know how
often, if ever, this is done in practice, but it looks like it would be
a problem. Two "nointegrity" mounts may have problem too. That should be
easy enough to verify.
Thanks,
Shaggy
>
> Thanks,
> Gu
>
>
> On 05/24/2013 04:57 PM, Vahram Martirosyan wrote:
>
>> In function jfs_freeze() the log is shut down through lmLogShutdown() call.
>> When the "nointegrity" mount option is enabled, the log is actually not
>> initialized. As a result the freeze operation in that case brings to a
>> kernel OOPS.
>>
>> The solution is to check if the "nointegrity" option is enabled and if it is not
>> then shut the log down.
>>
>> May be this is not the best solution, but at least it fixes the OOPS.
>>
>> Found by Linux File System Verification project (linuxtesting.org)
>>
>> Signed-off-by: Vahram Martirosyan <[email protected]>
>> ---
>> fs/jfs/super.c | 10 ++++++----
>> 1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/jfs/super.c b/fs/jfs/super.c
>> index a3d424d..9788970 100644
>> --- a/fs/jfs/super.c
>> +++ b/fs/jfs/super.c
>> @@ -615,10 +615,12 @@ static int jfs_freeze(struct super_block *sb)
>>
>> if (!(sb->s_flags & MS_RDONLY)) {
>> txQuiesce(sb);
>> - rc = lmLogShutdown(log);
>> - if (rc != 0) {
>> - jfs_err("lmLogShutdown failed with return code %d", rc);
>> - return rc;
>> + if (!log->no_integrity) {
>> + rc = lmLogShutdown(log);
>> + if (rc != 0) {
>> + jfs_err("lmLogShutdown failed with return code %d", rc);
>> + return rc;
>> + }
>> }
>> rc = updateSuper(sb, FM_CLEAN);
>> if (rc != 0) {
>
>
Vahram,
This is your first patch modified to be a little more robust. If there is
no objection, I'll push it to linux-next along with Gu's nointegrity
patch.
From: Vahram Martirosyan <[email protected]>
Date: Fri, 24 May 2013 13:57:12 +0500
The mentioned functions do not pay attention to the error codes returned
by the functions updateSuper(), lmLogInit() and lmLogShutdown(). It brings
to system crash later when writing to log.
The patch adds corresponding code to check and return the error codes
and to print correct error messages in case of errors.
Found by Linux File System Verification project (linuxtesting.org).
Signed-off-by: Vahram Martirosyan <[email protected]>
Reviewed-by: Gu Zheng <[email protected]>
Signed-off-by: Dave Kleikamp <[email protected]>
---
fs/jfs/super.c | 38 ++++++++++++++++++++++++++++++--------
1 file changed, 30 insertions(+), 8 deletions(-)
diff --git a/fs/jfs/super.c b/fs/jfs/super.c
index 2003e83..788e0a9 100644
--- a/fs/jfs/super.c
+++ b/fs/jfs/super.c
@@ -611,11 +611,28 @@ static int jfs_freeze(struct super_block *sb)
{
struct jfs_sb_info *sbi = JFS_SBI(sb);
struct jfs_log *log = sbi->log;
+ int rc = 0;
if (!(sb->s_flags & MS_RDONLY)) {
txQuiesce(sb);
- lmLogShutdown(log);
- updateSuper(sb, FM_CLEAN);
+ rc = lmLogShutdown(log);
+ if (rc) {
+ jfs_error(sb, "jfs_freeze: lmLogShutdown failed");
+
+ /* let operations fail rather than hang */
+ txResume(sb);
+
+ return rc;
+ }
+ rc = updateSuper(sb, FM_CLEAN);
+ if (rc) {
+ jfs_err("jfs_freeze: updateSuper failed\n");
+ /*
+ * Don't fail here. Everything succeeded except
+ * marking the superblock clean, so there's really
+ * no harm in leaving it frozen for now.
+ */
+ }
}
return 0;
}
@@ -627,13 +644,18 @@ static int jfs_unfreeze(struct super_block *sb)
int rc = 0;
if (!(sb->s_flags & MS_RDONLY)) {
- updateSuper(sb, FM_MOUNT);
- if ((rc = lmLogInit(log)))
- jfs_err("jfs_unlock failed with return code %d", rc);
- else
- txResume(sb);
+ rc = updateSuper(sb, FM_MOUNT);
+ if (rc) {
+ jfs_error(sb, "jfs_unfreeze: updateSuper failed");
+ goto out;
+ }
+ rc = lmLogInit(log);
+ if (rc)
+ jfs_error(sb, "jfs_unfreeze: lmLogInit failed");
+out:
+ txResume(sb);
}
- return 0;
+ return rc;
}
static struct dentry *jfs_do_mount(struct file_system_type *fs_type,
--
1.8.2.3