xfs
[Top] [All Lists]

Re: [PATCH] xfs: validate quota log items during log recovery

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH] xfs: validate quota log items during log recovery
From: Eric Sandeen <sandeen@xxxxxxxxxxx>
Date: Tue, 26 May 2009 11:07:56 -0500
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20090303175427.GA20582@xxxxxxxxxxxxx>
References: <20090303175427.GA20582@xxxxxxxxxxxxx>
User-agent: Thunderbird 2.0.0.21 (X11/20090320)
Christoph Hellwig wrote:
> Arkadiusz has been seeing really strange crashes in xfs_qm_dqcheck that
> I can only explain by a log item beeing too smal to actually fit the
                                 ^^being too small^^
> xfs_dqblk_t we're dereferencing all over xfs_qm_dqcheck.  So add
> graceful checks for NULL or too small quota items to the log recovery
> code.
> 
> 
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> 
> Index: xfs/fs/xfs/xfs_log_recover.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_log_recover.c 2009-03-02 04:15:11.410430892 +0100
> +++ xfs/fs/xfs/xfs_log_recover.c      2009-03-02 04:16:29.649444226 +0100
> @@ -1975,16 +1975,26 @@ xlog_recover_do_reg_buffer(
>               error = 0;
>               if (buf_f->blf_flags &
>                  (XFS_BLI_UDQUOT_BUF|XFS_BLI_PDQUOT_BUF|XFS_BLI_GDQUOT_BUF)) {
> +                     if (item->ri_buf[i].i_addr == NULL ||
> +                         item->ri_buf[i].i_len < sizeof(xfs_dqblk_t)) {
> +                             cmn_err(CE_ALERT,
> +     "XFS: dquot too small (%d) in xlog_recover_do_reg_buffer.",
> +                                             item->ri_buf[i].i_len);

Shouldn't this differentiate between i_addr == NULL and i_len too small,
though?  While we're at it anyway...

Maybe:

+       "XFS: dquot null addr (%p) or len too small (%d) in %s."
+       item->ri_buf[i].i_addr, item->ri_buf[i].i_len, __func__);

? (not hardcoding function name may be good too)

> +                             goto next;
> +                     }
>                       error = xfs_qm_dqcheck((xfs_disk_dquot_t *)
>                                              item->ri_buf[i].i_addr,
>                                              -1, 0, XFS_QMOPT_DOWARN,
>                                              "dquot_buf_recover");
> +                     if (error)
> +                             goto next;

I guess we can't do much else, but what happens in the end, when we skip
a buffer...

>               }
> -             if (!error)
> -                     memcpy(xfs_buf_offset(bp,
> -                             (uint)bit << XFS_BLI_SHIFT),    /* dest */
> -                             item->ri_buf[i].i_addr,         /* source */
> -                             nbits<<XFS_BLI_SHIFT);          /* length */
> +
> +             memcpy(xfs_buf_offset(bp,
> +                     (uint)bit << XFS_BLI_SHIFT),    /* dest */
> +                     item->ri_buf[i].i_addr,         /* source */
> +                     nbits<<XFS_BLI_SHIFT);          /* length */
> + next:
>               i++;
>               bit += nbits;
>       }
> @@ -2615,7 +2625,15 @@ xlog_recover_do_dquot_trans(
>               return (0);
>  
>       recddq = (xfs_disk_dquot_t *)item->ri_buf[1].i_addr;
> -     ASSERT(recddq);
> +
> +     if (item->ri_buf[1].i_addr == NULL ||
> +         item->ri_buf[1].i_len < sizeof(xfs_dqblk_t)) {
> +             cmn_err(CE_ALERT,
> +     "XFS: dquot too small (%d) in xlog_recover_do_dquot_trans.",
> +                     item->ri_buf[1].i_len);

Same deal here, should you differentiate on the error & use __func__ ?

-Eric

> +             return XFS_ERROR(EIO);
> +     }
> +
>       /*
>        * This type of quotas was turned off, so ignore this record.
>        */
> 
> _______________________________________________
> xfs mailing list
> xfs@xxxxxxxxxxx
> http://oss.sgi.com/mailman/listinfo/xfs
> 

<Prev in Thread] Current Thread [Next in Thread>