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:

quoted

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@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs