On Mon, Aug 17, 2015 at 01:52:18PM -0500, Eric Sandeen wrote: > On 8/14/15 8:43 PM, Darrick J. Wong wrote: > > If the user selects a corrupt inode via the 'inode XXX' command, the > > read verifier will fail and the io cursor at the top of the ring will > > not have any data attached. When this is the case, we cannot > > dereference the NULL pointer or xfs_db will crash. Therefore, check > > the buffer pointer before using it. > > > > It's arguable that we ought to retry the read without the verifiers > > if the inode is corrupt or fails CRC, since this /is/ a debugging > > tool, and maybe you wanted the contents anyway. > > I agree. It seems like we should do that, though it probably needs to > be done across the board for all metadata types if it's going to be done. > Maybe something to add to the TODO? Hmm. Looks like set_cur() wants to return buffer contents in the verifier error case, except that the broken xfs_buf->b_error sign handling prevents it from doing that... and the sign handling error is fixed later on in the patch set. So... that leaves hard IO errors, for which we still need this patch. --D > > > Signed-off-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx> > > --- > > db/inode.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/db/inode.c b/db/inode.c > > index e86dabd..64b263b 100644 > > --- a/db/inode.c > > +++ b/db/inode.c > > @@ -682,6 +682,8 @@ set_cur_inode( > > set_cur(&typtab[TYP_INODE], XFS_AGB_TO_DADDR(mp, agno, cluster_agbno), > > numblks, DB_RING_IGN, NULL); > > off_cur(offset << mp->m_sb.sb_inodelog, mp->m_sb.sb_inodesize); > > off_cur checks for iocur_top == NULL, and warns if it is, so that's good. > The user should have a clue about what's gone wrong, at least. > > But, callers of set_cur_inode() are still going to crash often as not: > > ablock_f: > set_cur_inode(iocur_top->ino); > haveattr = XFS_DFORK_Q((xfs_dinode_t *)iocur_top->data); > > bmap: > > set_cur_inode(iocur_top->ino); > nex = *nexp; > *nexp = 0; > ASSERT(nex > 0); > dip = iocur_top->data; > > bmap_f: > > set_cur_inode(iocur_top->ino); > dip = iocur_top->data; > > and a few more :( > > Perhaps set_cur_inode() should return failure, so the caller knows to bail, > pop_cur if it needs to, etc? > > -Eric > > > + if (!iocur_top->data) > > + return; > > dip = iocur_top->data; > > iocur_top->ino_buf = 1; > > iocur_top->ino = ino; > > > > _______________________________________________ > > xfs mailing list > > xfs@xxxxxxxxxxx > > http://oss.sgi.com/mailman/listinfo/xfs > > > > _______________________________________________ > xfs mailing list > xfs@xxxxxxxxxxx > http://oss.sgi.com/mailman/listinfo/xfs _______________________________________________ xfs mailing list xfs@xxxxxxxxxxx http://oss.sgi.com/mailman/listinfo/xfs
