Re: [PATCH] Make __xfs_xattr_put_listen preperly report errors.

[Dave Chinner]

On Fri, Aug 26, 2016 at 10:59:28AM +0200, Artem Savkov wrote:
> On Fri, Aug 26, 2016 at 08:42:15AM +1000, Dave Chinner wrote:
> > So when I look at the fix, and see that it doesn't reproduce on my
> > systems, it's clear that it's either not yet fully understood or
> > hasn't been fully explained by the person who understands the issue.
> > These are some of the questions I've asked myself to understand why
> > we are seeing what we've been seeing:
> > 
> > 	- what condition in the unfixed code leads to the ASSERT
> > 	  being tripped?
> > 	- how does the patch prevent that from occurring?
> > 	- at what threshold does the problem trigger (i.e. n=0, n=1,
> > 	  n=2 .... ?)
> > 	- how do the environmental initial conditions affect the
> > 	  test being run?
> > 	- what do security layers automatically store in the inode
> > 	  at creation time?
> > 	- how can we modify the test to always trigger the assert?
> > 
> > I know the answer, and it would take much less time to tell everyone
> > that it does to write an email like this.  But that means I'll just
> > have to do the same thing next time, and the next time, and so on.
> > The more people we have that can think through issues like this and
> > come to the right conclusion without needing my help, the better off
> > we'll all be...
> 
> Fair enough.
> 
> The problem only shows itself with a minimum of 2 xattrs and only when
> the buffer gets depleted before the last one.

This sentence needs to be in the commit description. :P

> LTP's llistxattr02 test
> only sets one xattr, but on my testsystem "security.selinux" attribute
> is automatically added on file creation which allows this bug to be
> reproduced. So I would assume that on your systems there are no
> automatically created xattrs and thats why you can't reproduce this.

On /some/ of my systems. I have a mix of selinux enabled/disabled
test machines, precisely because of the way always having an
attribute fork in the inode can perturb test results. I happened to
try to reproduce this on a machine that doesn't have selinux
enabled....

> Furthermore if buffersize is such that it is enough to hold the last
> xattr's name, but not enough to hold the sum of preceeding xattrs
> listxattr won't fail with ERANGE, but will suceed returning that xattr's
> name without the first character. The first character end's up
> overwriting whatever is stored at (context->alist - 1).

That should probably also be in the commit description - that
way when we have an idea of what problems it fixes when trying to
match upstream fixes to problems with older kernels (e.g. for distro
kernel backports).

Yes, I know it's a lot to put in a commit message, but in a couple
of years time nobody will remember these details. We regularly have
to work out why something was done 10-15 years ago in the code base,
and having good commit messages makes this a much easier job.
Someone like me will thank you in future for writing a comprehensive
commit message for a relatively simple bug fix....

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx