Re: [PATCH] fscrypt: Reduce object size of logging messages

[Joe Perches]

Hi again.

On Mon, 2020-09-07 at 15:23 -0700, Eric Biggers wrote:
> On Fri, Sep 04, 2020 at 09:38:23PM -0700, Joe Perches wrote:
> > > > diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c
> > > > index 9212325763b0..c82cc3907e43 100644
> > > > --- a/fs/crypto/crypto.c
> > > > +++ b/fs/crypto/crypto.c
> > > > @@ -329,25 +329,27 @@ int fscrypt_initialize(unsigned int cop_flags)
> > > >  	return err;
> > > >  }
> > > >  
> > > > -void fscrypt_msg(const struct inode *inode, const char *level,
> > > > -		 const char *fmt, ...)
> > > > +void fscrypt_printk(const struct inode *inode, const char *fmt, ...)
> > > >  {
> > > >  	static DEFINE_RATELIMIT_STATE(rs, DEFAULT_RATELIMIT_INTERVAL,
> > > >  				      DEFAULT_RATELIMIT_BURST);
> > > >  	struct va_format vaf;
> > > >  	va_list args;
> > > > +	int level;
> > > >  
> > > >  	if (!__ratelimit(&rs))
> > > >  		return;
> > > >  
> > > >  	va_start(args, fmt);
> > > > -	vaf.fmt = fmt;
> > > > +	level = printk_get_level(fmt);
> > > > +	vaf.fmt = printk_skip_level(fmt);
> > > >  	vaf.va = &args;
> > > >  	if (inode)
> > > > -		printk("%sfscrypt (%s, inode %lu): %pV\n",
> > > > -		       level, inode->i_sb->s_id, inode->i_ino, &vaf);
> > > > +		printk("%c%cfscrypt (%s, inode %lu): %pV\n",
> > > > +		       KERN_SOH_ASCII, level, inode->i_sb->s_id, inode->i_ino,
> > > > +		       &vaf);
> > > >  	else
> > > > -		printk("%sfscrypt: %pV\n", level, &vaf);
> > > > +		printk("%c%cfscrypt: %pV\n", KERN_SOH_ASCII, level, &vaf);
> > > >  	va_end(args);
> > > 
> > > The problem with this approach is that if fscrypt_printk() is called without
> > > providing a log level in the format string (which one would assume would work,
> > > since printk() allows it), then the real format string will be truncated to just
> > > KERN_SOH because 'level' will be 0.
> > > Can you find a way to avoid that?
> > 
> > While I don't think this is a problem in that all the fscrypt_<level>
> > calls will always prefix a KERN_<LEVEL>,
> 
> It's still a pitfall that people could run into later.  It would be better to
> make fscrypt_printk() work in the expected way.
> 
> > what btrfs uses:
> > 
> > 	char lvl[PRINTK_MAX_SINGLE_HEADER_LEN + 1] = "\0";
> > ...
> > 	while ((kern_level = printk_get_level(fmt)) != 0) {
> > 		size_t size = printk_skip_level(fmt) - fmt;
> > 
> > 		if (kern_level >= '0' && kern_level <= '7') {
> > 			memcpy(lvl, fmt,  size);
> > 			lvl[size] = '\0';
> > 		}
> > 		fmt += size;
> > 	}
> > 
> > and use "%s...", lvl, ...
> 
> Is the loop really needed?

It prevents defects (that btrfs had) where
btrfs_<level> used formats with KERN_<LEVEL>.

> > > > +#define fscrypt_warn(inode, fmt, ...)					\
> > > > +	fscrypt_printk(inode, KERN_WARNING fmt, ##__VA_ARGS__)
> > > 
> > > It's probably best to keep the parentheses around 'inode'.
> > 
> > Not really as it's an independent argument that can't
> > effectively have any other purpose but to be an argument
> > to the fsrypt_printk function.
> 
> True, but since forgetting to include parentheses around macro arguments is such
> a common mistake, IMO they should just always be included so that people don't
> have to think about whether the omission is correct or not.

We think differently.
Unnecessary parentheses are unnecessary and there
isn't a possiblity adding them here can be useful.