Re: [RFC 00/12] xfs: more and better verifiers

[Dave Chinner <david@fromorbit.com>]

On Fri, Aug 18, 2017 at 11:45:11AM -0700, Darrick J. Wong wrote:
> On Fri, Aug 18, 2017 at 10:06:07AM -0700, Darrick J. Wong wrote:
> > On Fri, Aug 18, 2017 at 12:05:16AM -0700, Christoph Hellwig wrote:
> > > On Thu, Aug 17, 2017 at 04:31:29PM -0700, Darrick J. Wong wrote:
> > > > Hi all,
> > > > 
> > > > This RFC combines all the random little fixes and improvements to the
> > > > verifiers that we've been talking about for the past month or so into a
> > > > single patch series!
> > > > 
> > > > We start by refactoring the long format btree block header verifier into
> > > > a single helper functionn and de-macroing dir block verifiers to make
> > > > them less shouty.  Next, we change verifier functions to return the
> > > > approximate instruction pointer of the faulting test so that we can
> > > > report more precise fault information to dmesg/tracepoints.
> > > 
> > > Just jumping here quickly because I don't have time for a detailed
> > > review:
> > > 
> > > How good does this instruction pointer thing resolved to the actual
> > > issue?
> > 
> > Ugh, it's terrible once you turn on the optimizer.
> > 
> >         if (!xfs_sb_version_hascrc(&mp->m_sb))                                  
> >                 return __this_address;                                          
> >         if (!uuid_equal(&block->bb_u.s.bb_uuid, &mp->m_sb.sb_meta_uuid))        
> >                 return __this_address;                                          
> >         if (block->bb_u.s.bb_blkno != cpu_to_be64(bp->b_bn))                    
> >                 return __this_address;                                          
> >         if (pag && be32_to_cpu(block->bb_u.s.bb_owner) != pag->pag_agno)        
> >                 return __this_address;                                          
> >         return NULL;                                                            
> > 
> > becomes:
> > 
> >         if (!xfs_sb_version_hascrc(&mp->m_sb))                                  
> >                 goto out;                                          
> >         if (!uuid_equal(&block->bb_u.s.bb_uuid, &mp->m_sb.sb_meta_uuid))        
> >                 goto out;                                          
> >         if (block->bb_u.s.bb_blkno != cpu_to_be64(bp->b_bn))                    
> >                 goto out;                                          
> >         if (pag && be32_to_cpu(block->bb_u.s.bb_owner) != pag->pag_agno)        
> >                 goto out;                                          
> >         return NULL;                                                            
> > out:
> > 	return __this_address;
> > 
> > ...which is totally worthless, unless we want to compile all the verifier
> > functions with __attribute__((optimize("O0"))), which is bogus.
> > 
> > <sigh> Back to the drawing board on that one.
> 
> Ok, there's /slightly/ less awful way to prevent gcc from optimizing the
> verifier function to the point of imprecise pointer value, but it involves
> writing to a volatile int:
> 
> /* stupidly prevent gcc from over-optimizing getting the instruction ptr */
> extern volatile int xfs_lineno;
> #define __this_address ({ __label__ __here; __here: xfs_lineno = __LINE__; &&__here; })
> 
> <grumble> Yucky, but it more or less works.

Can you declare the label as volatile, like you can an asm
statement to prevent the compiler from optimising out asm
statements?

Even so, given the yuckiness is very isolated and should only affect
the slow path code, I can live with this.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com