| Message ID | 20181105213145.7560-3-stefanrin@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Try to squash metadump data leaks | expand |
On Mon, Nov 05, 2018 at 10:31:42PM +0100, Stefan Ring wrote: > Using basically the same code as in process_single_fsb_objects. > > Signed-off-by: Stefan Ring <stefanrin@gmail.com> > --- > db/metadump.c | 19 ++++++++++++------- > 1 file changed, 12 insertions(+), 7 deletions(-) > > diff --git a/db/metadump.c b/db/metadump.c > index 97d2a490..be7cf360 100644 > --- a/db/metadump.c > +++ b/db/metadump.c > @@ -1881,6 +1881,7 @@ process_multi_fsb_objects( > typnm_t btype, > xfs_fileoff_t last) > { > + char *dp; > int ret = 0; > > switch (btype) { > @@ -1921,15 +1922,19 @@ process_multi_fsb_objects( > > } > > - if ((!obfuscate && !zero_stale_data) || Hmmm, if we're not obfuscating or zeroing data, can we still jump to write_buf()? --D > - o >= mp->m_dir_geo->leafblk) { > - ret = write_buf(iocur_top); > - goto out_pop; > + dp = iocur_top->data; > + if (o >= mp->m_dir_geo->freeblk) { > + /* TODO, zap any stale data */ > + goto write; > + } else if (o >= mp->m_dir_geo->leafblk) { > + process_dir_leaf_block(dp); > + } else { > + process_dir_data_block(dp, o, > + last == mp->m_dir_geo->fsbcount); > } > > - process_dir_data_block(iocur_top->data, o, > - last == mp->m_dir_geo->fsbcount); > - iocur_top->need_crc = 1; > + iocur_top->need_crc = obfuscate || zero_stale_data; > +write: > ret = write_buf(iocur_top); > out_pop: > pop_cur(); > -- > 2.14.5 >
On Thu, Jan 3, 2019 at 6:55 PM Darrick J. Wong <darrick.wong@oracle.com> wrote: > On Mon, Nov 05, 2018 at 10:31:42PM +0100, Stefan Ring wrote: > > Using basically the same code as in process_single_fsb_objects. > > > > Signed-off-by: Stefan Ring <stefanrin@gmail.com> > > --- > > db/metadump.c | 19 ++++++++++++------- > > 1 file changed, 12 insertions(+), 7 deletions(-) > > > > diff --git a/db/metadump.c b/db/metadump.c > > index 97d2a490..be7cf360 100644 > > --- a/db/metadump.c > > +++ b/db/metadump.c > > @@ -1921,15 +1922,19 @@ process_multi_fsb_objects( > > > > } > > > > - if ((!obfuscate && !zero_stale_data) || > > Hmmm, if we're not obfuscating or zeroing data, can we still jump to > write_buf()? > > --D The call to write_buf() is always necessary, as without this, nothing would end up in the metadump. But while trying to argue the soundness of this patch, I have found deficiencies ;). It will always call process_dir_data_block, even without any zeroing/obfuscation flags set. I must have considered this harmless, since it does not actually "do" anything, but I'm not so sure anymore because it may complain about data inconsistencies, and this might make an otherwise dumpable image non-dumpable. It is also, at least in theory, able to reset the need_crc back to zero, contrary to all the other usages of this flag in this file. I don't think this is an actual problem, as the flag only seems to be set when zeroing or obfuscating in the first place, but it might become one if circumstances far removed from this location change. It is just too much of a mental burden. I will redo this once again so that it will be more similar in structure to the code in process_single_fsb_objects _and_ the original code, I hope. > > > - o >= mp->m_dir_geo->leafblk) { > > - ret = write_buf(iocur_top); > > - goto out_pop; > > + dp = iocur_top->data; > > + if (o >= mp->m_dir_geo->freeblk) { > > + /* TODO, zap any stale data */ > > + goto write; > > + } else if (o >= mp->m_dir_geo->leafblk) { > > + process_dir_leaf_block(dp); > > + } else { > > + process_dir_data_block(dp, o, > > + last == mp->m_dir_geo->fsbcount); > > } > > > > - process_dir_data_block(iocur_top->data, o, > > - last == mp->m_dir_geo->fsbcount); > > - iocur_top->need_crc = 1; > > + iocur_top->need_crc = obfuscate || zero_stale_data; > > +write: > > ret = write_buf(iocur_top); > > out_pop: > > pop_cur(); > > -- > > 2.14.5 > >
On 1/4/19 4:27 PM, Stefan Ring wrote: > On Thu, Jan 3, 2019 at 6:55 PM Darrick J. Wong <darrick.wong@oracle.com> wrote: >> On Mon, Nov 05, 2018 at 10:31:42PM +0100, Stefan Ring wrote: >>> Using basically the same code as in process_single_fsb_objects. >>> >>> Signed-off-by: Stefan Ring <stefanrin@gmail.com> >>> --- >>> db/metadump.c | 19 ++++++++++++------- >>> 1 file changed, 12 insertions(+), 7 deletions(-) >>> >>> diff --git a/db/metadump.c b/db/metadump.c >>> index 97d2a490..be7cf360 100644 >>> --- a/db/metadump.c >>> +++ b/db/metadump.c >>> @@ -1921,15 +1922,19 @@ process_multi_fsb_objects( >>> >>> } >>> >>> - if ((!obfuscate && !zero_stale_data) || >> >> Hmmm, if we're not obfuscating or zeroing data, can we still jump to >> write_buf()? >> >> --D > > The call to write_buf() is always necessary, as without this, nothing > would end up in the metadump. But while trying to argue the soundness > of this patch, I have found deficiencies ;). It will always call > process_dir_data_block, even without any zeroing/obfuscation flags > set. Right, I think that was Darrick's point. (I had noticed this too but have been woefully slow in replying.) > I must have considered this harmless, since it does not actually > "do" anything, but I'm not so sure anymore because it may complain > about data inconsistencies, and this might make an otherwise dumpable > image non-dumpable. Yup, exactly. You're good! ;) > It is also, at least in theory, able to reset the > need_crc back to zero, contrary to all the other usages of this flag > in this file. I don't think this is an actual problem, as the flag > only seems to be set when zeroing or obfuscating in the first place, > but it might become one if circumstances far removed from this > location change. It is just too much of a mental burden. I feel like the whole need_crc business could be cleaned up, TBH (later). > I will redo this once again so that it will be more similar in > structure to the code in process_single_fsb_objects _and_ the original > code, I hope. FWIW, I've been running all of xfstests in various permutations with a patch to dump and restore and xfs_repair an image after each run, using each of "" "-o" "-a" and "-oa" as metadump arguments. Some corruptions have appeared, but I don't yet know if they're regressions or not. (i.e. the filesystem was intact but the restored metadump was not.) I'll let the run finish and re-run the failed tests w/ stock code and see what I find. -Eric
diff --git a/db/metadump.c b/db/metadump.c index 97d2a490..be7cf360 100644 --- a/db/metadump.c +++ b/db/metadump.c @@ -1881,6 +1881,7 @@ process_multi_fsb_objects( typnm_t btype, xfs_fileoff_t last) { + char *dp; int ret = 0; switch (btype) { @@ -1921,15 +1922,19 @@ process_multi_fsb_objects( } - if ((!obfuscate && !zero_stale_data) || - o >= mp->m_dir_geo->leafblk) { - ret = write_buf(iocur_top); - goto out_pop; + dp = iocur_top->data; + if (o >= mp->m_dir_geo->freeblk) { + /* TODO, zap any stale data */ + goto write; + } else if (o >= mp->m_dir_geo->leafblk) { + process_dir_leaf_block(dp); + } else { + process_dir_data_block(dp, o, + last == mp->m_dir_geo->fsbcount); } - process_dir_data_block(iocur_top->data, o, - last == mp->m_dir_geo->fsbcount); - iocur_top->need_crc = 1; + iocur_top->need_crc = obfuscate || zero_stale_data; +write: ret = write_buf(iocur_top); out_pop: pop_cur();
Using basically the same code as in process_single_fsb_objects. Signed-off-by: Stefan Ring <stefanrin@gmail.com> --- db/metadump.c | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-)