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 _______________________________________________ xfs mailing list xfs@xxxxxxxxxxx http://oss.sgi.com/mailman/listinfo/xfs
