Patchwork [00/10] LP#922906: oops in ticket_spin_lock

login
register
mail settings
Submitter Andy Whitcroft
Date April 16, 2012, 6:02 p.m.
Message ID <1334599360-15346-1-git-send-email-apw@canonical.com>
Download mbox
Permalink /patch/152956/
State New
Headers show

Pull-request

git://kernel.ubuntu.com/apw/ubuntu-precise.git lp922906

Comments

Andy Whitcroft - April 16, 2012, 6:02 p.m.
We have been seeing a large number of reports of oopses in
ticket_spin_lock, these are mostly occuring in inotify related calls.
There seems to be several races in this code and there is a fairly
substantial patch set upstream to reorder the locking and sort this out.
I have pulled these patches back to precise from the notify upstream tree,
the patches themselves are slated for the 3.5 merge window.

Patches follow, pull request below for those who prefer to look at them
locally.

I am proposing them (somewhat reluctantly due to their size) for Precise.

Comments?

-apw

The following changes since commit 48853edab3b414cc73c3e1c037f477524053cf15:

  fsnotify: change locking order (2012-04-16 18:33:10 +0100)

are available in the git repository at:

  git://kernel.ubuntu.com/apw/ubuntu-precise.git lp922906

for you to fetch changes up to 48853edab3b414cc73c3e1c037f477524053cf15:

  fsnotify: change locking order (2012-04-16 18:33:10 +0100)

----------------------------------------------------------------

Lino Sanfilippo (10):
  inotify, fanotify: replace fsnotify_put_group() with
    fsnotify_destroy_group()
  fsnotify: introduce fsnotify_get_group()
  fsnotify: use reference counting for groups
  fsnotify: take groups mark_lock before mark lock
  fanotify: add an extra flag to mark_remove_from_mask that indicates
    wheather a mark should be destroyed
  fsnotify: use a mutex instead of a spinlock to protect a groups mark
    list
  fsnotify: pass group to fsnotify_destroy_mark()
  fsnotify: introduce locked versions of fsnotify_add_mark() and
    fsnotify_remove_mark()
  fsnotify: dont put marks on temporary list when clearing marks by
    group
  fsnotify: change locking order

 fs/notify/dnotify/dnotify.c          |    4 +-
 fs/notify/fanotify/fanotify_user.c   |   33 +++++++-----
 fs/notify/group.c                    |   40 +++++++--------
 fs/notify/inode_mark.c               |   14 ++++--
 fs/notify/inotify/inotify_fsnotify.c |    4 +-
 fs/notify/inotify/inotify_user.c     |   11 ++--
 fs/notify/mark.c                     |   91 +++++++++++++++++++---------------
 fs/notify/vfsmount_mark.c            |   14 ++++--
 include/linux/fsnotify_backend.h     |   26 ++++++----
 kernel/audit_tree.c                  |   10 ++--
 kernel/audit_watch.c                 |    4 +-
 11 files changed, 147 insertions(+), 104 deletions(-)
Luis Henriques - April 16, 2012, 7:29 p.m.
On Mon, Apr 16, 2012 at 07:02:30PM +0100, Andy Whitcroft wrote:
> We have been seeing a large number of reports of oopses in
> ticket_spin_lock, these are mostly occuring in inotify related calls.
> There seems to be several races in this code and there is a fairly
> substantial patch set upstream to reorder the locking and sort this out.
> I have pulled these patches back to precise from the notify upstream tree,
> the patches themselves are slated for the 3.5 merge window.
> 
> Patches follow, pull request below for those who prefer to look at them
> locally.
> 
> I am proposing them (somewhat reluctantly due to their size) for Precise.
> 
> Comments?

Just a comment: I do believe that this patch should also close LP#951537.
I never marked that as a duplicate of LP#922906, but it looks like the
same issue, with the racy fsnotify invocations.
 
> -apw
> 
> The following changes since commit 48853edab3b414cc73c3e1c037f477524053cf15:
> 
>   fsnotify: change locking order (2012-04-16 18:33:10 +0100)
> 
> are available in the git repository at:
> 
>   git://kernel.ubuntu.com/apw/ubuntu-precise.git lp922906
> 
> for you to fetch changes up to 48853edab3b414cc73c3e1c037f477524053cf15:
> 
>   fsnotify: change locking order (2012-04-16 18:33:10 +0100)
> 
> ----------------------------------------------------------------
> 
> Lino Sanfilippo (10):
>   inotify, fanotify: replace fsnotify_put_group() with
>     fsnotify_destroy_group()
>   fsnotify: introduce fsnotify_get_group()
>   fsnotify: use reference counting for groups
>   fsnotify: take groups mark_lock before mark lock
>   fanotify: add an extra flag to mark_remove_from_mask that indicates
>     wheather a mark should be destroyed
>   fsnotify: use a mutex instead of a spinlock to protect a groups mark
>     list
>   fsnotify: pass group to fsnotify_destroy_mark()
>   fsnotify: introduce locked versions of fsnotify_add_mark() and
>     fsnotify_remove_mark()
>   fsnotify: dont put marks on temporary list when clearing marks by
>     group
>   fsnotify: change locking order
> 
>  fs/notify/dnotify/dnotify.c          |    4 +-
>  fs/notify/fanotify/fanotify_user.c   |   33 +++++++-----
>  fs/notify/group.c                    |   40 +++++++--------
>  fs/notify/inode_mark.c               |   14 ++++--
>  fs/notify/inotify/inotify_fsnotify.c |    4 +-
>  fs/notify/inotify/inotify_user.c     |   11 ++--
>  fs/notify/mark.c                     |   91 +++++++++++++++++++---------------
>  fs/notify/vfsmount_mark.c            |   14 ++++--
>  include/linux/fsnotify_backend.h     |   26 ++++++----
>  kernel/audit_tree.c                  |   10 ++--
>  kernel/audit_watch.c                 |    4 +-
>  11 files changed, 147 insertions(+), 104 deletions(-)
> 
> -- 
> 1.7.9.5
> 
> 
> -- 
> kernel-team mailing list
> kernel-team@lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team

Cheers,
--
Luis
Brad Figg - April 16, 2012, 7:55 p.m.
On 04/16/2012 11:02 AM, Andy Whitcroft wrote:
> We have been seeing a large number of reports of oopses in
> ticket_spin_lock, these are mostly occuring in inotify related calls.
> There seems to be several races in this code and there is a fairly
> substantial patch set upstream to reorder the locking and sort this out.
> I have pulled these patches back to precise from the notify upstream tree,
> the patches themselves are slated for the 3.5 merge window.
> 
> Patches follow, pull request below for those who prefer to look at them
> locally.
> 
> I am proposing them (somewhat reluctantly due to their size) for Precise.
> 
> Comments?
> 
> -apw
> 
> The following changes since commit 48853edab3b414cc73c3e1c037f477524053cf15:
> 
>   fsnotify: change locking order (2012-04-16 18:33:10 +0100)
> 
> are available in the git repository at:
> 
>   git://kernel.ubuntu.com/apw/ubuntu-precise.git lp922906
> 
> for you to fetch changes up to 48853edab3b414cc73c3e1c037f477524053cf15:
> 
>   fsnotify: change locking order (2012-04-16 18:33:10 +0100)
> 
> ----------------------------------------------------------------
> 
> Lino Sanfilippo (10):
>   inotify, fanotify: replace fsnotify_put_group() with
>     fsnotify_destroy_group()
>   fsnotify: introduce fsnotify_get_group()
>   fsnotify: use reference counting for groups
>   fsnotify: take groups mark_lock before mark lock
>   fanotify: add an extra flag to mark_remove_from_mask that indicates
>     wheather a mark should be destroyed
>   fsnotify: use a mutex instead of a spinlock to protect a groups mark
>     list
>   fsnotify: pass group to fsnotify_destroy_mark()
>   fsnotify: introduce locked versions of fsnotify_add_mark() and
>     fsnotify_remove_mark()
>   fsnotify: dont put marks on temporary list when clearing marks by
>     group
>   fsnotify: change locking order
> 
>  fs/notify/dnotify/dnotify.c          |    4 +-
>  fs/notify/fanotify/fanotify_user.c   |   33 +++++++-----
>  fs/notify/group.c                    |   40 +++++++--------
>  fs/notify/inode_mark.c               |   14 ++++--
>  fs/notify/inotify/inotify_fsnotify.c |    4 +-
>  fs/notify/inotify/inotify_user.c     |   11 ++--
>  fs/notify/mark.c                     |   91 +++++++++++++++++++---------------
>  fs/notify/vfsmount_mark.c            |   14 ++++--
>  include/linux/fsnotify_backend.h     |   26 ++++++----
>  kernel/audit_tree.c                  |   10 ++--
>  kernel/audit_watch.c                 |    4 +-
>  11 files changed, 147 insertions(+), 104 deletions(-)
> 

I've looked these over and I don't see any obvious problems.

Are we proposing to take these before they are in either Linus' or the upstream
stable trees?

Problems I have taking them now:
1. Obviously they have not have the kind of upstream review that we require for
   most patches.
2. The patches won't have received a great deal of testing from upstream. However,
   people should recognise that most of these patches don't get any kind of real
   testing until they are picked up by a distro.
3. The number of changes and the type of changes (locking and reference counting).

I'm supposed to be the one that guards the stability of the kernels (especially
LTS kernels) and be more conservative that most. However, I'm inclined to take
these patches, get them out and tested as best we can and as early in the support
period that we can so that if there are issues, they are found and fixed as
quickly as possible.

Brad
Stefan Bader - April 17, 2012, 9:25 a.m.
On 16.04.2012 20:02, Andy Whitcroft wrote:
> We have been seeing a large number of reports of oopses in
> ticket_spin_lock, these are mostly occuring in inotify related calls.
> There seems to be several races in this code and there is a fairly
> substantial patch set upstream to reorder the locking and sort this out.
> I have pulled these patches back to precise from the notify upstream tree,
> the patches themselves are slated for the 3.5 merge window.
> 
> Patches follow, pull request below for those who prefer to look at them
> locally.
> 
> I am proposing them (somewhat reluctantly due to their size) for Precise.
> 
> Comments?
> 
> -apw
> 
> The following changes since commit 48853edab3b414cc73c3e1c037f477524053cf15:
> 
>   fsnotify: change locking order (2012-04-16 18:33:10 +0100)
> 
> are available in the git repository at:
> 
>   git://kernel.ubuntu.com/apw/ubuntu-precise.git lp922906
> 
> for you to fetch changes up to 48853edab3b414cc73c3e1c037f477524053cf15:
> 
>   fsnotify: change locking order (2012-04-16 18:33:10 +0100)
> 
> ----------------------------------------------------------------
> 
> Lino Sanfilippo (10):
>   inotify, fanotify: replace fsnotify_put_group() with
>     fsnotify_destroy_group()
>   fsnotify: introduce fsnotify_get_group()
>   fsnotify: use reference counting for groups
>   fsnotify: take groups mark_lock before mark lock
>   fanotify: add an extra flag to mark_remove_from_mask that indicates
>     wheather a mark should be destroyed
>   fsnotify: use a mutex instead of a spinlock to protect a groups mark
>     list
>   fsnotify: pass group to fsnotify_destroy_mark()
>   fsnotify: introduce locked versions of fsnotify_add_mark() and
>     fsnotify_remove_mark()
>   fsnotify: dont put marks on temporary list when clearing marks by
>     group
>   fsnotify: change locking order
> 
>  fs/notify/dnotify/dnotify.c          |    4 +-
>  fs/notify/fanotify/fanotify_user.c   |   33 +++++++-----
>  fs/notify/group.c                    |   40 +++++++--------
>  fs/notify/inode_mark.c               |   14 ++++--
>  fs/notify/inotify/inotify_fsnotify.c |    4 +-
>  fs/notify/inotify/inotify_user.c     |   11 ++--
>  fs/notify/mark.c                     |   91 +++++++++++++++++++---------------
>  fs/notify/vfsmount_mark.c            |   14 ++++--
>  include/linux/fsnotify_backend.h     |   26 ++++++----
>  kernel/audit_tree.c                  |   10 ++--
>  kernel/audit_watch.c                 |    4 +-
>  11 files changed, 147 insertions(+), 104 deletions(-)
> 

There is as always two sides: the rules of stable sanity would rather say we
should wait till the whole series is upstream and has a certain time to settle
before considering this.
Looking through the patchset which is large and touches mainly reference counts
and locking (the natural sources of madness), there is still a certain feeling
of nervousness about 3 and 8+9. It is hard to decide those things are safe or
not without knowing the whole code by heart.
On the other hand the locking of inotify seems to be broken a lot based on the
many bug reports and duplicates. Or at least the problems are quite likely to be
hit by a lot of people based on a common workflow. So it would be good to fix
things sooner than later.
If we do that, then I agree with Brad, better early. And maybe trying to be
prepared with a revert-this emergency upload in the sleve... For some reason
people always find the serious problems _after_ moving into updates...

-Stefan