Hi Brian,
On Fri, Sep 04, 2020 at 07:25:29AM -0400, Brian Foster wrote:
> On Fri, Sep 04, 2020 at 04:25:15PM +0800, Gao Xiang wrote:
...
> > @@ -2904,9 +2904,10 @@ STATIC int
> > xlog_valid_rec_header(
> > struct xlog *log,
> > struct xlog_rec_header *rhead,
> > - xfs_daddr_t blkno)
> > + xfs_daddr_t blkno,
> > + int bufsize)
> > {
> > - int hlen;
> > + int hlen, hsize = XLOG_BIG_RECORD_BSIZE;
> >
> > if (XFS_IS_CORRUPT(log->l_mp,
> > rhead->h_magicno != cpu_to_be32(XLOG_HEADER_MAGIC_NUM)))
> > @@ -2920,10 +2921,22 @@ xlog_valid_rec_header(
> > return -EFSCORRUPTED;
> > }
> >
> > - /* LR body must have data or it wouldn't have been written */
> > + /*
> > + * LR body must have data (or it wouldn't have been written) and
> > + * h_len must not be greater than h_size with one exception (see
> > + * comments in xlog_do_recovery_pass()).
> > + */
>
> I wouldn't mention the exceptional case at all here since I think it
> just adds confusion. It's an unfortunate wart with mkfs that requires a
> kernel workaround, and I think it's better to keep it one place. I.e.,
> should it ever be removed, I find it unlikely somebody will notice this
> comment and fix it up accordingly.
Thanks for your review.
ok, if I understand correctly, will remove this "with one exception
(see comments..." expression. Please kindly correct me if I
misunderstand.
>
> > +
> > + if (XFS_IS_CORRUPT(log->l_mp, hlen <= 0 || hlen > hsize))
> > return -EFSCORRUPTED;
> > +
> > + if (bufsize && XFS_IS_CORRUPT(log->l_mp, bufsize < hsize))
> > + return -EFSCORRUPTED;
>
> Please do something like the following so the full corruption check
> logic is readable:
>
> if (XFS_IS_CORRUPT(..., bufsize && hsize > bufsize))
> return -EFSCORRUPTED;
That is good idea, will update this as well.
>
...
> > rhead = (xlog_rec_header_t *)offset;
> > - error = xlog_valid_rec_header(log, rhead, tail_blk);
> > - if (error)
> > - goto bread_err1;
>
> This technically defers broader corruption checks (i.e., magic number,
> etc.) until after functional code starts using other fields below. I
> don't think we should remove this.
>
I'm trying to combine this with the following part...(see below...)
> >
> > /*
> > * xfsprogs has a bug where record length is based on lsunit but
> > @@ -3001,21 +3011,19 @@ xlog_do_recovery_pass(
> > */
> > h_size = be32_to_cpu(rhead->h_size);
> > h_len = be32_to_cpu(rhead->h_len);
> > - if (h_len > h_size) {
> > - if (h_len <= log->l_mp->m_logbsize &&
> > - be32_to_cpu(rhead->h_num_logops) == 1) {
> > - xfs_warn(log->l_mp,
> > + if (h_len > h_size && h_len <= log->l_mp->m_logbsize &&
> > + rhead->h_num_logops == cpu_to_be32(1)) {
> > + xfs_warn(log->l_mp,
> > "invalid iclog size (%d bytes), using lsunit (%d bytes)",
> > - h_size, log->l_mp->m_logbsize);
> > - h_size = log->l_mp->m_logbsize;
> > - } else {
> > - XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW,
> > - log->l_mp);
> > - error = -EFSCORRUPTED;
> > - goto bread_err1;
> > - }
> > + h_size, log->l_mp->m_logbsize);
> > + h_size = log->l_mp->m_logbsize;
> > + rhead->h_size = cpu_to_be32(h_size);
>
> I don't think we should update rhead like this, particularly in a rare
> and exclusive case. This structure should reflect what is on disk.
>
> All in all, I think this patch should be much more focused:
>
> 1.) Add the bufsize parameter and associated corruption check to
> xlog_valid_rec_header().
> 2.) Pass the related value from the existing calls.
> 3.) (Optional) If there's reason to revalidate after executing the mkfs
> workaround, add a second call within the branch that implements the
> h_size workaround.
>
I moved workaround code to xlog_valid_rec_header() at first is
because in xlog_valid_rec_header() actually it has 2 individual
checks now:
1) check rhead->h_len vs rhead->h_size for each individual log record;
2) check rhead->h_size vs the unique allocated buffer size passed in
for each record (since each log record has one stored h_size,
even though there are not used later according to the current
logic of xlog_do_recovery_pass).
if any of the conditions above is not satisfied, xlog_valid_rec_header()
will make fs corrupted immediately, so I tried 2 ways up to now:
- (v1,v2) fold in workaround case into xlog_valid_rec_header()
- (v3) rearrange workaround and xlog_valid_rec_header() order in
xlog_do_recovery_pass() and modify rhead->h_size to the
workaround h_size before xlog_valid_rec_header() validation
so xlog_valid_rec_header() will work as expected since it
has two individual checks as mentioned above.
If there is some better way, kindly let me know :) and I'd like to
hear other folks about this in advance as well.... so I can go
forward since this part is a bit tricky for now.
> Also, please test the workaround case to make sure it still works as
> expected (if you haven't already).
ok, will double confirm this, thanks!
Thanks,
Gao Xiang
>
> Brian
>
