[lkml]   [2016]   [Aug]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [PATCH] Make __xfs_xattr_put_listen preperly report errors.
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

> 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....


Dave Chinner

 \ /
  Last update: 2016-09-17 09:58    [W:0.047 / U:0.660 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site