| Message ID | 20170411141237.9274-2-jtulak@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Tue, Apr 11, 2017 at 04:12:36PM +0200, Jan Tulak wrote: > Add a warning about possible corruption when exporting a dirty log, as > the log content does not agree with obfuscated metadata. > > Signed-off-by: Jan Tulak <jtulak@redhat.com> > --- Thanks for posting this... > db/metadump.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/db/metadump.c b/db/metadump.c > index 66952f6..74e24b2 100644 > --- a/db/metadump.c > +++ b/db/metadump.c > @@ -2726,7 +2726,8 @@ copy_log(void) > /* keep the dirty log */ > if (obfuscate) > print_warning( > -_("Filesystem log is dirty; image will contain unobfuscated metadata in log.")); > +_("Filesystem log is dirty; image will contain unobfuscated metadata in log " > + "and a log replay can cause a corruption.")); I think a slightly more verbose message might be a good idea. For example, something like the following: "Filesystem log is dirty; image will contain unobfuscated metadata in the log. Log recovery of an obfuscated image can cause filesystem corruption. Please mount the source image to clean the log or disable metadump obfuscation." That could also say "... or verify that log recovery of the resulting image does not cause corruption," but that might be overkill. Thoughts? Eric? Brian > break; > case -1: > /* log detection error */ > -- > 2.1.4 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 4/11/17 1:30 PM, Brian Foster wrote: > On Tue, Apr 11, 2017 at 04:12:36PM +0200, Jan Tulak wrote: >> Add a warning about possible corruption when exporting a dirty log, as >> the log content does not agree with obfuscated metadata. >> >> Signed-off-by: Jan Tulak <jtulak@redhat.com> >> --- > > Thanks for posting this... > >> db/metadump.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/db/metadump.c b/db/metadump.c >> index 66952f6..74e24b2 100644 >> --- a/db/metadump.c >> +++ b/db/metadump.c >> @@ -2726,7 +2726,8 @@ copy_log(void) >> /* keep the dirty log */ >> if (obfuscate) >> print_warning( >> -_("Filesystem log is dirty; image will contain unobfuscated metadata in log.")); >> +_("Filesystem log is dirty; image will contain unobfuscated metadata in log " >> + "and a log replay can cause a corruption.")); > > I think a slightly more verbose message might be a good idea. For > example, something like the following: > > "Filesystem log is dirty; image will contain unobfuscated metadata in > the log. Log recovery of an obfuscated image can cause filesystem > corruption. Please mount the source image to clean the log or disable > metadump obfuscation." > > That could also say "... or verify that log recovery of the resulting > image does not cause corruption," but that might be overkill. Thoughts? > Eric? I think we do need a good explanation, but that will take a lot of workd. We could also refer to the man page for more details - it's getting pretty long for a warning from the tool. The other bummer is that we cannot detect whether an image has been obfuscated, so the restore tool can only say "if it's obfuscated...." -Eric > Brian > >> break; >> case -1: >> /* log detection error */ >> -- >> 2.1.4 >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Apr 11, 2017 at 01:34:25PM -0500, Eric Sandeen wrote: > On 4/11/17 1:30 PM, Brian Foster wrote: > > On Tue, Apr 11, 2017 at 04:12:36PM +0200, Jan Tulak wrote: > >> Add a warning about possible corruption when exporting a dirty log, as > >> the log content does not agree with obfuscated metadata. > >> > >> Signed-off-by: Jan Tulak <jtulak@redhat.com> > >> --- > > > > Thanks for posting this... > > > >> db/metadump.c | 3 ++- > >> 1 file changed, 2 insertions(+), 1 deletion(-) > >> > >> diff --git a/db/metadump.c b/db/metadump.c > >> index 66952f6..74e24b2 100644 > >> --- a/db/metadump.c > >> +++ b/db/metadump.c > >> @@ -2726,7 +2726,8 @@ copy_log(void) > >> /* keep the dirty log */ > >> if (obfuscate) > >> print_warning( > >> -_("Filesystem log is dirty; image will contain unobfuscated metadata in log.")); > >> +_("Filesystem log is dirty; image will contain unobfuscated metadata in log " > >> + "and a log replay can cause a corruption.")); > > > > I think a slightly more verbose message might be a good idea. For > > example, something like the following: > > > > "Filesystem log is dirty; image will contain unobfuscated metadata in > > the log. Log recovery of an obfuscated image can cause filesystem > > corruption. Please mount the source image to clean the log or disable > > metadump obfuscation." > > > > That could also say "... or verify that log recovery of the resulting > > image does not cause corruption," but that might be overkill. Thoughts? > > Eric? > > I think we do need a good explanation, but that will take a lot of workd. > We could also refer to the man page for more details - it's getting pretty > long for a warning from the tool. > Hm, yeah. Maybe the existing warning can be condensed a bit more to something like: "Warning: log recovery of an obfuscated metadata image can leak unobfuscated metadata and/or cause filesystem corruption. Please mount the source image to clean the log or disable obfuscation." I suppose we could also just exit under such conditions unless the user passes a force flag or something. Maybe that's overkill too, though.. Brian > The other bummer is that we cannot detect whether an image has been > obfuscated, so the restore tool can only say "if it's obfuscated...." > > -Eric > > > Brian > > > >> break; > >> case -1: > >> /* log detection error */ > >> -- > >> 2.1.4 > >> > >> -- > >> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > >> the body of a message to majordomo@vger.kernel.org > >> More majordomo info at http://vger.kernel.org/majordomo-info.html > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 4/11/17 1:43 PM, Brian Foster wrote: > On Tue, Apr 11, 2017 at 01:34:25PM -0500, Eric Sandeen wrote: >> On 4/11/17 1:30 PM, Brian Foster wrote: >>> On Tue, Apr 11, 2017 at 04:12:36PM +0200, Jan Tulak wrote: >>>> Add a warning about possible corruption when exporting a dirty log, as >>>> the log content does not agree with obfuscated metadata. >>>> >>>> Signed-off-by: Jan Tulak <jtulak@redhat.com> >>>> --- >>> >>> Thanks for posting this... >>> >>>> db/metadump.c | 3 ++- >>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/db/metadump.c b/db/metadump.c >>>> index 66952f6..74e24b2 100644 >>>> --- a/db/metadump.c >>>> +++ b/db/metadump.c >>>> @@ -2726,7 +2726,8 @@ copy_log(void) >>>> /* keep the dirty log */ >>>> if (obfuscate) >>>> print_warning( >>>> -_("Filesystem log is dirty; image will contain unobfuscated metadata in log.")); >>>> +_("Filesystem log is dirty; image will contain unobfuscated metadata in log " >>>> + "and a log replay can cause a corruption.")); >>> >>> I think a slightly more verbose message might be a good idea. For >>> example, something like the following: >>> >>> "Filesystem log is dirty; image will contain unobfuscated metadata in >>> the log. Log recovery of an obfuscated image can cause filesystem >>> corruption. Please mount the source image to clean the log or disable >>> metadump obfuscation." >>> >>> That could also say "... or verify that log recovery of the resulting >>> image does not cause corruption," but that might be overkill. Thoughts? >>> Eric? >> >> I think we do need a good explanation, but that will take a lot of workd. >> We could also refer to the man page for more details - it's getting pretty >> long for a warning from the tool. >> > > Hm, yeah. Maybe the existing warning can be condensed a bit more to > something like: > > "Warning: log recovery of an obfuscated metadata image can leak > unobfuscated metadata and/or cause filesystem corruption. Please mount > the source image to clean the log or disable obfuscation." s/filesystem corruption/image corruption/ - we don't want anyone to think that it damaged the original fs! > I suppose we could also just exit under such conditions unless the user > passes a force flag or something. Maybe that's overkill too, though.. yeah, let's not overengineer - dumping a dirty, obfuscated log is quite typical I think. -Eric -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Apr 11, 2017 at 02:01:41PM -0500, Eric Sandeen wrote: > On 4/11/17 1:43 PM, Brian Foster wrote: > > On Tue, Apr 11, 2017 at 01:34:25PM -0500, Eric Sandeen wrote: > >> On 4/11/17 1:30 PM, Brian Foster wrote: > >>> On Tue, Apr 11, 2017 at 04:12:36PM +0200, Jan Tulak wrote: > >>>> Add a warning about possible corruption when exporting a dirty log, as > >>>> the log content does not agree with obfuscated metadata. > >>>> > >>>> Signed-off-by: Jan Tulak <jtulak@redhat.com> > >>>> --- > >>> > >>> Thanks for posting this... > >>> > >>>> db/metadump.c | 3 ++- > >>>> 1 file changed, 2 insertions(+), 1 deletion(-) > >>>> > >>>> diff --git a/db/metadump.c b/db/metadump.c > >>>> index 66952f6..74e24b2 100644 > >>>> --- a/db/metadump.c > >>>> +++ b/db/metadump.c > >>>> @@ -2726,7 +2726,8 @@ copy_log(void) > >>>> /* keep the dirty log */ > >>>> if (obfuscate) > >>>> print_warning( > >>>> -_("Filesystem log is dirty; image will contain unobfuscated metadata in log.")); > >>>> +_("Filesystem log is dirty; image will contain unobfuscated metadata in log " > >>>> + "and a log replay can cause a corruption.")); > >>> > >>> I think a slightly more verbose message might be a good idea. For > >>> example, something like the following: > >>> > >>> "Filesystem log is dirty; image will contain unobfuscated metadata in > >>> the log. Log recovery of an obfuscated image can cause filesystem > >>> corruption. Please mount the source image to clean the log or disable > >>> metadump obfuscation." > >>> > >>> That could also say "... or verify that log recovery of the resulting > >>> image does not cause corruption," but that might be overkill. Thoughts? > >>> Eric? > >> > >> I think we do need a good explanation, but that will take a lot of workd. > >> We could also refer to the man page for more details - it's getting pretty > >> long for a warning from the tool. > >> > > > > Hm, yeah. Maybe the existing warning can be condensed a bit more to > > something like: > > > > "Warning: log recovery of an obfuscated metadata image can leak > > unobfuscated metadata and/or cause filesystem corruption. Please mount > > the source image to clean the log or disable obfuscation." > > s/filesystem corruption/image corruption/ - we don't want anyone to think > that it damaged the original fs! OTOH the fs might be damaged just badly enough that log recovery is impossible (which is why we're creating the metadump to send to support) so aborting metadump is the wrong thing to do. It seems sort of silly even to lecture the user about log recovery when they might not be able to recover said log and might not ever even start the log recovery process. --D > > > I suppose we could also just exit under such conditions unless the user > > passes a force flag or something. Maybe that's overkill too, though.. > > yeah, let's not overengineer - dumping a dirty, obfuscated log is quite > typical I think. > > -Eric > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Apr 11, 2017 at 04:44:44PM -0700, Darrick J. Wong wrote: > On Tue, Apr 11, 2017 at 02:01:41PM -0500, Eric Sandeen wrote: > > On 4/11/17 1:43 PM, Brian Foster wrote: > > > On Tue, Apr 11, 2017 at 01:34:25PM -0500, Eric Sandeen wrote: > > >> On 4/11/17 1:30 PM, Brian Foster wrote: > > >>> On Tue, Apr 11, 2017 at 04:12:36PM +0200, Jan Tulak wrote: > > >>>> Add a warning about possible corruption when exporting a dirty log, as > > >>>> the log content does not agree with obfuscated metadata. > > >>>> > > >>>> Signed-off-by: Jan Tulak <jtulak@redhat.com> > > >>>> --- > > >>> > > >>> Thanks for posting this... > > >>> > > >>>> db/metadump.c | 3 ++- > > >>>> 1 file changed, 2 insertions(+), 1 deletion(-) > > >>>> > > >>>> diff --git a/db/metadump.c b/db/metadump.c > > >>>> index 66952f6..74e24b2 100644 > > >>>> --- a/db/metadump.c > > >>>> +++ b/db/metadump.c > > >>>> @@ -2726,7 +2726,8 @@ copy_log(void) > > >>>> /* keep the dirty log */ > > >>>> if (obfuscate) > > >>>> print_warning( > > >>>> -_("Filesystem log is dirty; image will contain unobfuscated metadata in log.")); > > >>>> +_("Filesystem log is dirty; image will contain unobfuscated metadata in log " > > >>>> + "and a log replay can cause a corruption.")); > > >>> > > >>> I think a slightly more verbose message might be a good idea. For > > >>> example, something like the following: > > >>> > > >>> "Filesystem log is dirty; image will contain unobfuscated metadata in > > >>> the log. Log recovery of an obfuscated image can cause filesystem > > >>> corruption. Please mount the source image to clean the log or disable > > >>> metadump obfuscation." > > >>> > > >>> That could also say "... or verify that log recovery of the resulting > > >>> image does not cause corruption," but that might be overkill. Thoughts? > > >>> Eric? > > >> > > >> I think we do need a good explanation, but that will take a lot of workd. > > >> We could also refer to the man page for more details - it's getting pretty > > >> long for a warning from the tool. > > >> > > > > > > Hm, yeah. Maybe the existing warning can be condensed a bit more to > > > something like: > > > > > > "Warning: log recovery of an obfuscated metadata image can leak > > > unobfuscated metadata and/or cause filesystem corruption. Please mount > > > the source image to clean the log or disable obfuscation." > > > > s/filesystem corruption/image corruption/ - we don't want anyone to think > > that it damaged the original fs! > > OTOH the fs might be damaged just badly enough that log recovery is > impossible (which is why we're creating the metadump to send to support) > so aborting metadump is the wrong thing to do. > Indeed.. > It seems sort of silly even to lecture the user about log recovery when > they might not be able to recover said log and might not ever even start > the log recovery process. > There's a bit of a balance here between handling dirty log + obfuscation cases where a log recovery issue is the purpose of the metadump, it is not the purpose and the original fs can run log recovery without disrupting problem diagnosis, or something in between and the resulting image is explicitly verified to not lead to such metadump-specific corruption. I think adjusting the preexisting warning wrt to the fact that log data is not obfuscated to point out this issue is a relatively minor change given the potential result.. (put another way, it seems kind of silly that we'd warn about leaking unobfuscated data but not this..). Brian > --D > > > > > > I suppose we could also just exit under such conditions unless the user > > > passes a force flag or something. Maybe that's overkill too, though.. > > > > yeah, let's not overengineer - dumping a dirty, obfuscated log is quite > > typical I think. > > > > -Eric > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Apr 12, 2017 at 1:03 PM, Brian Foster <bfoster@redhat.com> wrote: > On Tue, Apr 11, 2017 at 04:44:44PM -0700, Darrick J. Wong wrote: >> On Tue, Apr 11, 2017 at 02:01:41PM -0500, Eric Sandeen wrote: >> > On 4/11/17 1:43 PM, Brian Foster wrote: >> > > On Tue, Apr 11, 2017 at 01:34:25PM -0500, Eric Sandeen wrote: >> > >> On 4/11/17 1:30 PM, Brian Foster wrote: >> > >>> On Tue, Apr 11, 2017 at 04:12:36PM +0200, Jan Tulak wrote: >> > >>>> Add a warning about possible corruption when exporting a dirty log, as >> > >>>> the log content does not agree with obfuscated metadata. >> > >>>> >> > >>>> Signed-off-by: Jan Tulak <jtulak@redhat.com> >> > >>>> --- >> > >>> >> > >>> Thanks for posting this... >> > >>> >> > >>>> db/metadump.c | 3 ++- >> > >>>> 1 file changed, 2 insertions(+), 1 deletion(-) >> > >>>> >> > >>>> diff --git a/db/metadump.c b/db/metadump.c >> > >>>> index 66952f6..74e24b2 100644 >> > >>>> --- a/db/metadump.c >> > >>>> +++ b/db/metadump.c >> > >>>> @@ -2726,7 +2726,8 @@ copy_log(void) >> > >>>> /* keep the dirty log */ >> > >>>> if (obfuscate) >> > >>>> print_warning( >> > >>>> -_("Filesystem log is dirty; image will contain unobfuscated metadata in log.")); >> > >>>> +_("Filesystem log is dirty; image will contain unobfuscated metadata in log " >> > >>>> + "and a log replay can cause a corruption.")); >> > >>> >> > >>> I think a slightly more verbose message might be a good idea. For >> > >>> example, something like the following: >> > >>> >> > >>> "Filesystem log is dirty; image will contain unobfuscated metadata in >> > >>> the log. Log recovery of an obfuscated image can cause filesystem >> > >>> corruption. Please mount the source image to clean the log or disable >> > >>> metadump obfuscation." >> > >>> >> > >>> That could also say "... or verify that log recovery of the resulting >> > >>> image does not cause corruption," but that might be overkill. Thoughts? >> > >>> Eric? >> > >> >> > >> I think we do need a good explanation, but that will take a lot of workd. >> > >> We could also refer to the man page for more details - it's getting pretty >> > >> long for a warning from the tool. >> > >> >> > > >> > > Hm, yeah. Maybe the existing warning can be condensed a bit more to >> > > something like: >> > > >> > > "Warning: log recovery of an obfuscated metadata image can leak >> > > unobfuscated metadata and/or cause filesystem corruption. Please mount >> > > the source image to clean the log or disable obfuscation." >> > >> > s/filesystem corruption/image corruption/ - we don't want anyone to think >> > that it damaged the original fs! >> >> OTOH the fs might be damaged just badly enough that log recovery is >> impossible (which is why we're creating the metadump to send to support) >> so aborting metadump is the wrong thing to do. >> > > Indeed.. > >> It seems sort of silly even to lecture the user about log recovery when >> they might not be able to recover said log and might not ever even start >> the log recovery process. >> > > There's a bit of a balance here between handling dirty log + obfuscation > cases where a log recovery issue is the purpose of the metadump, it is > not the purpose and the original fs can run log recovery without > disrupting problem diagnosis, or something in between and the resulting > image is explicitly verified to not lead to such metadump-specific > corruption. > > I think adjusting the preexisting warning wrt to the fact that log data > is not obfuscated to point out this issue is a relatively minor change > given the potential result.. (put another way, it seems kind of silly > that we'd warn about leaking unobfuscated data but not this..). > > Brian > I think that this is something we really should notify about. It serves as a notification for a rare issue, "hey, are you aware of this," rather than a list of instructions or a manual. So, I would keep the message short. This (slightly modified) variant sounds good to me: "Warning: log recovery of an obfuscated metadata image can leak unobfuscated metadata and/or cause image corruption. Please mount the source image to clean the log or disable obfuscation, if possible." Jan
diff --git a/db/metadump.c b/db/metadump.c index 66952f6..74e24b2 100644 --- a/db/metadump.c +++ b/db/metadump.c @@ -2726,7 +2726,8 @@ copy_log(void) /* keep the dirty log */ if (obfuscate) print_warning( -_("Filesystem log is dirty; image will contain unobfuscated metadata in log.")); +_("Filesystem log is dirty; image will contain unobfuscated metadata in log " + "and a log replay can cause a corruption.")); break; case -1: /* log detection error */
Add a warning about possible corruption when exporting a dirty log, as the log content does not agree with obfuscated metadata. Signed-off-by: Jan Tulak <jtulak@redhat.com> --- db/metadump.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)