| Message ID | 20120713142839.23587.58658.stgit@localhost.localdomain (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 07/13/2012 09:28 AM, Alan Cox wrote: > From: Alan Cox <alan@linux.intel.com> > > We re-run the loop but we don't re-set the attrs pointer back to NULL. It looks to me like we're OK here without this. At the top of the loop, the if condition either holds or it does not. - If it does not, we don't touch "xattrs" again, before returning "err". - If the condition holds, the next time "xattrs" is touched is when its value is assigned the result of a kcalloc() call. That being said, I really do prefer to have pointers get invalidated after their freed, so I'll happily add your change... Reviewed-by: Alex Elder <elder@inktank.com> > Signed-off-by: Alan Cox <alan@linux.intel.com> > --- > > fs/ceph/xattr.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c > index 785cb30..2c2ae5b 100644 > --- a/fs/ceph/xattr.c > +++ b/fs/ceph/xattr.c > @@ -457,6 +457,7 @@ start: > for (i = 0; i < numattr; i++) > kfree(xattrs[i]); > kfree(xattrs); > + xattrs = NULL; > goto start; > } > err = -EIO; > > -- > To unsubscribe from this list: send the line "unsubscribe ceph-devel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > > -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 13 Jul 2012 09:36:57 -0500 Alex Elder <elder@inktank.com> wrote: > On 07/13/2012 09:28 AM, Alan Cox wrote: > > From: Alan Cox <alan@linux.intel.com> > > > > We re-run the loop but we don't re-set the attrs pointer back to NULL. > > It looks to me like we're OK here without this. > > At the top of the loop, the if condition either holds or it does not. > - If it does not, we don't touch "xattrs" again, before returning "err". > - If the condition holds, the next time "xattrs" is touched is when its > value is assigned the result of a kcalloc() call. If the condition is invariant across the race/retry then a better fix would be to move the start label to re-execute only the relevant code ? Alan -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/13/2012 09:56 AM, Alan Cox wrote: > On Fri, 13 Jul 2012 09:36:57 -0500 > Alex Elder <elder@inktank.com> wrote: > >> On 07/13/2012 09:28 AM, Alan Cox wrote: >>> From: Alan Cox <alan@linux.intel.com> >>> >>> We re-run the loop but we don't re-set the attrs pointer back to NULL. >> >> It looks to me like we're OK here without this. >> >> At the top of the loop, the if condition either holds or it does not. >> - If it does not, we don't touch "xattrs" again, before returning "err". >> - If the condition holds, the next time "xattrs" is touched is when its >> value is assigned the result of a kcalloc() call. > > > If the condition is invariant across the race/retry then a better fix > would be to move the start label to re-execute only the relevant code ? I haven't looked at this code in a while so I might be missing something. Let's see if I can explain what's going on to see if it clears anything up. The condition is not invariant across the race. The spin lock ci->i_ceph_lock is held on entry to the function, and that protects the ci->i_xattrs structure, including updates to its "blob" field. The xattrs "blob" is a buffer that holds a copy of the on-disk representation of the extended attributes. There is a red-black tree "index" version of that information also. Whether the index is up-to-date with what's on disk is determined by whether the value of index_version is at or above the value of the (on-disk) version of the xattrs. This block of code is re-building red-black tree index. The "if" statement at the top is determining whether there actually is anything to build (iov_len <= 4 means there are no attributes in the blob). The "xattrs" variable your patch affects holds an array of structures holding xattr information that will be referred to by entries in the red-black tree (index). Its size is determined by the content of the "blob". We drop the lock while allocating the "xattrs" array, and as a result the "blob" could change, meaning the size of the "xattrs" array we allocated could be wrong after re-acquiring the lock. This is checked by seeing if the version changed, and if so, the array is freed and we start over, by jumping back to the "start" label. Is that clear? Is there something I'm still missing? I do have some changes I've been sitting on that clean this stuff up a bit but I'm not sure they affect this particular spot in the code. In any case, as I said, I plan to include your proposed fix. -Alex -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> Is that clear? Is there something I'm still missing?
Basically if they are not invariant I don't see why it can't go around
the loop, allocate the buffer, free it and then the next time find there
is nothing there and thus double free.
Either way if its patched the problem goes away so it's mostly for my own
understanding.
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/13/2012 11:37 AM, Alan Cox wrote: >> Is that clear? Is there something I'm still missing? > > Basically if they are not invariant I don't see why it can't go around > the loop, allocate the buffer, free it and then the next time find there > is nothing there and thus double free. > > Either way if its patched the problem goes away so it's mostly for my own > understanding. The key is that xattrs is a local variable I think. 1) enter the "if" block 2) spin_unlock() 3) xattrs = kcalloc()... 4) spin_lock() 5) version changes, so: 6) kfree everything (now xattrs is invalid) 7) goto start Then either: 8a) re-enter the "if" block 9a) spin_unlock() 10a) xattrs = kcalloc()... <- now xattrs is valid again . . . Or: 8b) do not enter the "if" block 9b) return err... <- xattrs is not referenced again -Alex -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c index 785cb30..2c2ae5b 100644 --- a/fs/ceph/xattr.c +++ b/fs/ceph/xattr.c @@ -457,6 +457,7 @@ start: for (i = 0; i < numattr; i++) kfree(xattrs[i]); kfree(xattrs); + xattrs = NULL; goto start; } err = -EIO;