Unexpected behaviour of idr_get_empty_slot() on parisc

Unexpected behaviour of idr_get_empty_slot() on parisc

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1674 bytes --]

Ryan Niebur reported in <http://bugs.debian.org/561880> that on a
PA-RISC system running Linux 2.6.32 the following test case for inotify:

#include <sys/inotify.h>
#include <stdio.h>

int main()
{
    int fd, wd;

    fd = inotify_init ();
    wd = inotify_add_watch (fd,
                            "test.c",
                            IN_OPEN | IN_CLOSE);
    printf("First: %d\n", wd);
    inotify_rm_watch(fd, wd);
    wd = inotify_add_watch (fd,
                            "a.out",
                            IN_OPEN | IN_CLOSE);
    printf("Second: %d\n", wd);
    inotify_rm_watch(fd, wd);
}

produces the output:

First: 1
Second: 1

The behaviour he (and the test case's author) expects is that ids for
closed descriptors are not reused and the second id will be 2.  The
implementation does seem to ensure that:

static int inotify_handle_get_wd(struct inotify_handle *ih,
				 struct inotify_watch *watch)
{
	int ret;

	do {
		if (unlikely(!idr_pre_get(&ih->idr, GFP_NOFS)))
			return -ENOSPC;
		ret = idr_get_new_above(&ih->idr, watch, ih->last_wd+1, &watch->wd);
	} while (ret == -EAGAIN);

	if (likely(!ret))
		ih->last_wd = watch->wd;

	return ret;
}

Removing the inotify_rm_watch() calls from the test case results in the
expected output, so I have no reason to think that id can be assigned
while it is already in use.  However, it does appear that
idr_get_empty_slot() can somehow select an id smaller than starting_id.

Ben.

-- 
Ben Hutchings
The obvious mathematical breakthrough [to break modern encryption] would be
development of an easy way to factor large prime numbers. - Bill Gates

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: Unexpected behaviour of idr_get_empty_slot() on parisc

[Kyle McMartin]

On Sun, Jan 03, 2010 at 11:23:25PM +0000, Ben Hutchings wrote:
> Ryan Niebur reported in <http://bugs.debian.org/561880> that on a
> PA-RISC system running Linux 2.6.32 the following test case for inotify:
> 

I bet this is because classic RCU is removed, and tree RCU was
insufficiently tested on crap platforms before the removal.

I'll try to re-add it and we can see if it's an RCU problem or something
nastier.

Re: Unexpected behaviour of idr_get_empty_slot() on parisc

[James Bottomley]

On Sun, 2010-01-03 at 18:50 -0500, Kyle McMartin wrote:
> On Sun, Jan 03, 2010 at 11:23:25PM +0000, Ben Hutchings wrote:
> > Ryan Niebur reported in <http://bugs.debian.org/561880> that on a
> > PA-RISC system running Linux 2.6.32 the following test case for inotify:
> > 
> 
> I bet this is because classic RCU is removed, and tree RCU was
> insufficiently tested on crap platforms before the removal.
> 
> I'll try to re-add it and we can see if it's an RCU problem or something
> nastier.

Apparently it's a feature of kernels beyond 2.6.30 (this exact
behaviour).  Suspect the bug report is incomplete and the i386 machine
was running 2.6.29 or lower.

Red Hat has been getting complaints about it so they're looking to
revert to the old 2.6.29 behaviour as well.  The actual commit that
caused the behaviour change is

commit 63c882a05416e18de6fb59f7dd6da48f3bbe8273
Author: Eric Paris <eparis@redhat.com>
Date:   Thu May 21 17:02:01 2009 -0400

    inotify: reimplement inotify using fsnotify

James

Re: Unexpected behaviour of idr_get_empty_slot() on parisc

[James Bottomley]

On Fri, 2010-01-15 at 10:26 -0600, James Bottomley wrote:
> On Sun, 2010-01-03 at 18:50 -0500, Kyle McMartin wrote:
> > On Sun, Jan 03, 2010 at 11:23:25PM +0000, Ben Hutchings wrote:
> > > Ryan Niebur reported in <http://bugs.debian.org/561880> that on a
> > > PA-RISC system running Linux 2.6.32 the following test case for inotify:
> > > 
> > 
> > I bet this is because classic RCU is removed, and tree RCU was
> > insufficiently tested on crap platforms before the removal.
> > 
> > I'll try to re-add it and we can see if it's an RCU problem or something
> > nastier.
> 
> Apparently it's a feature of kernels beyond 2.6.30 (this exact
> behaviour).  Suspect the bug report is incomplete and the i386 machine
> was running 2.6.29 or lower.
> 
> Red Hat has been getting complaints about it so they're looking to
> revert to the old 2.6.29 behaviour as well.  The actual commit that
> caused the behaviour change is
> 
> commit 63c882a05416e18de6fb59f7dd6da48f3bbe8273
> Author: Eric Paris <eparis@redhat.com>
> Date:   Thu May 21 17:02:01 2009 -0400
> 
>     inotify: reimplement inotify using fsnotify

Just to close this out, the fix (for all architectures) is now upstream:

commit 9e572cc9877ee6c43af60778f6b8d5ba0692d935
Author: Eric Paris <eparis@redhat.com>
Date:   Fri Jan 15 12:12:24 2010 -0500

    inotify: do not reuse watch descriptors

James