All of lore.kernel.org
 help / color / mirror / Atom feed
From: Wu Fengguang <fengguang.wu@intel.com>
To: Andrew Morton <akpm@linux-foundation.org>,
	Kay Sievers <kay.sievers@vrfy.org>
Cc: "linux-nfs@vger.kernel.org" <linux-nfs@vger.kernel.org>,
	Trond Myklebust <Trond.Myklebust@netapp.com>,
	"linux-hotplug@vger.kernel.org" <linux-hotplug@vger.kernel.org>,
	Eric Paris <eparis@redhat.com>, Al Viro <viro@zeniv.linux.org.uk>,
	Christoph Hellwig <hch@lst.de>
Subject: [PATCH] inotify: report rounded-up event size to user space
Date: Thu, 07 May 2009 11:59:47 +0000	[thread overview]
Message-ID: <20090507115947.GA20934@localhost> (raw)
In-Reply-To: <ac3eb2510905060642o41f9c68rf1138e94d76d611c@mail.gmail.com>

On Wed, May 06, 2009 at 09:42:58PM +0800, Kay Sievers wrote:
> On Mon, May 4, 2009 at 15:30, Wu Fengguang <fengguang.wu@intel.com> wrote:
> 
> > I tried remove every udev rules in /etc/udev/ and /lib/udev, the /etc/group
> > accesses disappeared in strace, but udevd is still busy.
> 
> > ppoll([{fd=4, events=POLLIN}, {fd=5, events=POLLIN}, {fd=3, events=POLLIN}], 3, NULL, [], 8) = 1 ([{fd=3, revents=POLLIN}])
> > ioctl(3, FIONREAD, [39])                = 0
> > read(3, 0x62ad60, 39)                   = -1 EINVAL (Invalid argument)
> 
> Seems, you have issues with inotify on your nfs mount?
> 
> Inotify wakes up udevd to tell something in the rules directory has
> changed, but inotify seems not to return anything useful, but keeps
> waking us up. That causes an endless loop of parsing rules files.

Thanks for the tip. The failed inotify read() is caused by the size *roundup*
behavior introduced by the -mm commit 3b46cf7d5f3ca(Reimplement inotify_user
using fsnotify).  Which says:

+               /*
+                * We need to pad the filename so as to properly align an
+                * array of inotify_event structures.  Because the structure is
+                * small and the common case is a small filename, we just round
+                * up to the next multiple of the structure's sizeof.  This is
+                * simple and safe for all architectures.
+                */

The udev madness originates from these kernel testing failures:

[  756.569243] get_one_event: event_sizeH > count8, name_len", namea-dev-root-link.rules
[  756.600103] get_one_event: event_sizeH > count8, name_len", namea-dev-root-link.rules
[  756.630265] get_one_event: event_sizeH > count8, name_len", namea-dev-root-link.rules
[  756.670862] get_one_event: event_sizeH > count8, name_len", namea-dev-root-link.rules
[  756.701845] get_one_event: event_sizeH > count8, name_len", namea-dev-root-link.rules
[  756.732899] get_one_event: event_sizeH > count8, name_len", namea-dev-root-link.rules
[  756.763126] get_one_event: event_sizeH > count8, name_len", namea-dev-root-link.rules
[  756.794829] get_one_event: event_sizeH > count8, name_len", namea-dev-root-link.rules
[  756.824985] get_one_event: event_sizeH > count8, name_len", namea-dev-root-link.rules
[  756.856760] get_one_event: event_sizeH > count8, name_len", namea-dev-root-link.rules
[  761.608521] __ratelimit: 210 callbacks suppressed

Which are printed by the following patch:

--- a/fs/notify/inotify/inotify_user.c
+++ b/fs/notify/inotify/inotify_user.c
 static struct fsnotify_event *get_one_event(struct fsnotify_group *group,
 					    size_t count)
 {
 	size_t event_size = sizeof(struct inotify_event);
 	struct fsnotify_event *event;
 
 	if (fsnotify_notify_queue_is_empty(group))
 		return NULL;
 
 	event = fsnotify_peek_notify_event(group);
 
 	event_size += roundup(event->name_len, event_size);
 
-	if (event_size > count)
+	if (event_size > count) {
+		if (printk_ratelimit())
+			printk("get_one_event: event_size=%d > count=%d, name_len=%d, name=%s\n",
+					(int)event_size, (int)count, (int)event->name_len, event->file_name);
 		return ERR_PTR(-EINVAL);
+	}


It can be fixed by reporting the rounded up value to user space.

Thanks,
Fengguang
---
inotify: report rounded-up event size to user space

Fix a udev madness problem, which falls into an endless loop:

(1) ppoll([{fd=4, events=POLLIN}, {fd=5, events=POLLIN}, {fd=3, events=POLLIN}], 3, NULL, [], 8) = 1 ([{fd=3, revents=POLLIN}])
(2) ioctl(3, FIONREAD, [39])                = 0
(3) read(3, 0x62ad60, 39)                   = -1 EINVAL (Invalid argument)

In (2) we reported a small len, while in (3) we insist on a rounded up len,
leading to a failed inotify read(), which will be retried endlessly by udev.

[  756.569243] get_one_event: event_sizeH > count8, name_len", namea-dev-root-link.rules
[  756.600103] get_one_event: event_sizeH > count8, name_len", namea-dev-root-link.rules
[  756.630265] get_one_event: event_sizeH > count8, name_len", namea-dev-root-link.rules
[  756.670862] get_one_event: event_sizeH > count8, name_len", namea-dev-root-link.rules
[  756.701845] get_one_event: event_sizeH > count8, name_len", namea-dev-root-link.rules
[  756.732899] get_one_event: event_sizeH > count8, name_len", namea-dev-root-link.rules
[  756.763126] get_one_event: event_sizeH > count8, name_len", namea-dev-root-link.rules
[  756.794829] get_one_event: event_sizeH > count8, name_len", namea-dev-root-link.rules
[  756.824985] get_one_event: event_sizeH > count8, name_len", namea-dev-root-link.rules
[  756.856760] get_one_event: event_sizeH > count8, name_len", namea-dev-root-link.rules
[  761.608521] __ratelimit: 210 callbacks suppressed

Signed-off-by: Wu Fengguang <fengguang.wu@intel.com>
---
 fs/notify/inotify/inotify_user.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

--- linux.orig/fs/notify/inotify/inotify_user.c
+++ linux/fs/notify/inotify/inotify_user.c
@@ -318,7 +318,9 @@ static long inotify_ioctl(struct file *f
 		mutex_lock(&group->notification_mutex);
 		list_for_each_entry(holder, &group->notification_list, event_list) {
 			event = holder->event;
-			send_len += sizeof(struct inotify_event) + event->name_len;
+			send_len += sizeof(struct inotify_event);
+			send_len += roundup(event->name_len,
+					sizeof(struct inotify_event));
 		}
 		mutex_unlock(&group->notification_mutex);
 		ret = put_user(send_len, (int __user *) p);

WARNING: multiple messages have this Message-ID (diff)
From: Wu Fengguang <fengguang.wu@intel.com>
To: Andrew Morton <akpm@linux-foundation.org>,
	Kay Sievers <kay.sievers@vrfy.org>
Cc: "linux-nfs@vger.kernel.org" <linux-nfs@vger.kernel.org>,
	Trond Myklebust <Trond.Myklebust@netapp.com>,
	"linux-hotplug@vger.kernel.org" <linux-hotplug@vger.kernel.org>,
	Eric Paris <eparis@redhat.com>, Al Viro <viro@zeniv.linux.org.uk>,
	Christoph Hellwig <hch@lst.de>
Subject: [PATCH] inotify: report rounded-up event size to user space
Date: Thu, 7 May 2009 19:59:47 +0800	[thread overview]
Message-ID: <20090507115947.GA20934@localhost> (raw)
In-Reply-To: <ac3eb2510905060642o41f9c68rf1138e94d76d611c@mail.gmail.com>

On Wed, May 06, 2009 at 09:42:58PM +0800, Kay Sievers wrote:
> On Mon, May 4, 2009 at 15:30, Wu Fengguang <fengguang.wu@intel.com> w=
rote:
>=20
> > I tried remove every udev rules in /etc/udev/ and /lib/udev, the /e=
tc/group
> > accesses disappeared in strace, but udevd is still busy.
>=20
> > ppoll([{fd=3D4, events=3DPOLLIN}, {fd=3D5, events=3DPOLLIN}, {fd=3D=
3, events=3DPOLLIN}], 3, NULL, [], 8) =3D 1 ([{fd=3D3, revents=3DPOLLIN=
}])
> > ioctl(3, FIONREAD, [39]) =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0=3D 0
> > read(3, 0x62ad60, 39) =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =3D -1 EINVAL (Invalid argument)
>=20
> Seems, you have issues with inotify on your nfs mount?
>=20
> Inotify wakes up udevd to tell something in the rules directory has
> changed, but inotify seems not to return anything useful, but keeps
> waking us up. That causes an endless loop of parsing rules files.

Thanks for the tip. The failed inotify read() is caused by the size *ro=
undup*
behavior introduced by the -mm commit 3b46cf7d5f3ca(Reimplement inotify=
_user
using fsnotify).  Which says:

+               /*
+                * We need to pad the filename so as to properly align =
an
+                * array of inotify_event structures.  Because the stru=
cture is
+                * small and the common case is a small filename, we ju=
st round
+                * up to the next multiple of the structure's sizeof.  =
This is
+                * simple and safe for all architectures.
+                */

The udev madness originates from these kernel testing failures:

[  756.569243] get_one_event: event_size=3D48 > count=3D38, name_len=3D=
22, name=3D61-dev-root-link.rules
[  756.600103] get_one_event: event_size=3D48 > count=3D38, name_len=3D=
22, name=3D61-dev-root-link.rules
[  756.630265] get_one_event: event_size=3D48 > count=3D38, name_len=3D=
22, name=3D61-dev-root-link.rules
[  756.670862] get_one_event: event_size=3D48 > count=3D38, name_len=3D=
22, name=3D61-dev-root-link.rules
[  756.701845] get_one_event: event_size=3D48 > count=3D38, name_len=3D=
22, name=3D61-dev-root-link.rules
[  756.732899] get_one_event: event_size=3D48 > count=3D38, name_len=3D=
22, name=3D61-dev-root-link.rules
[  756.763126] get_one_event: event_size=3D48 > count=3D38, name_len=3D=
22, name=3D61-dev-root-link.rules
[  756.794829] get_one_event: event_size=3D48 > count=3D38, name_len=3D=
22, name=3D61-dev-root-link.rules
[  756.824985] get_one_event: event_size=3D48 > count=3D38, name_len=3D=
22, name=3D61-dev-root-link.rules
[  756.856760] get_one_event: event_size=3D48 > count=3D38, name_len=3D=
22, name=3D61-dev-root-link.rules
[  761.608521] __ratelimit: 210 callbacks suppressed

Which are printed by the following patch:

--- a/fs/notify/inotify/inotify_user.c
+++ b/fs/notify/inotify/inotify_user.c
 static struct fsnotify_event *get_one_event(struct fsnotify_group *gro=
up,
 					    size_t count)
 {
 	size_t event_size =3D sizeof(struct inotify_event);
 	struct fsnotify_event *event;
=20
 	if (fsnotify_notify_queue_is_empty(group))
 		return NULL;
=20
 	event =3D fsnotify_peek_notify_event(group);
=20
 	event_size +=3D roundup(event->name_len, event_size);
=20
-	if (event_size > count)
+	if (event_size > count) {
+		if (printk_ratelimit())
+			printk("get_one_event: event_size=3D%d > count=3D%d, name_len=3D%d,=
 name=3D%s\n",
+					(int)event_size, (int)count, (int)event->name_len, event->file_na=
me);
 		return ERR_PTR(-EINVAL);
+	}


It can be fixed by reporting the rounded up value to user space.

Thanks,
=46engguang
---
inotify: report rounded-up event size to user space

=46ix a udev madness problem, which falls into an endless loop:

(1) ppoll([{fd=3D4, events=3DPOLLIN}, {fd=3D5, events=3DPOLLIN}, {fd=3D=
3, events=3DPOLLIN}], 3, NULL, [], 8) =3D 1 ([{fd=3D3, revents=3DPOLLIN=
}])
(2) ioctl(3, FIONREAD, [39]) =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0=3D 0
(3) read(3, 0x62ad60, 39) =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =3D -1 EINVAL (Invalid argument)

In (2) we reported a small len, while in (3) we insist on a rounded up =
len,
leading to a failed inotify read(), which will be retried endlessly by =
udev.

[  756.569243] get_one_event: event_size=3D48 > count=3D38, name_len=3D=
22, name=3D61-dev-root-link.rules
[  756.600103] get_one_event: event_size=3D48 > count=3D38, name_len=3D=
22, name=3D61-dev-root-link.rules
[  756.630265] get_one_event: event_size=3D48 > count=3D38, name_len=3D=
22, name=3D61-dev-root-link.rules
[  756.670862] get_one_event: event_size=3D48 > count=3D38, name_len=3D=
22, name=3D61-dev-root-link.rules
[  756.701845] get_one_event: event_size=3D48 > count=3D38, name_len=3D=
22, name=3D61-dev-root-link.rules
[  756.732899] get_one_event: event_size=3D48 > count=3D38, name_len=3D=
22, name=3D61-dev-root-link.rules
[  756.763126] get_one_event: event_size=3D48 > count=3D38, name_len=3D=
22, name=3D61-dev-root-link.rules
[  756.794829] get_one_event: event_size=3D48 > count=3D38, name_len=3D=
22, name=3D61-dev-root-link.rules
[  756.824985] get_one_event: event_size=3D48 > count=3D38, name_len=3D=
22, name=3D61-dev-root-link.rules
[  756.856760] get_one_event: event_size=3D48 > count=3D38, name_len=3D=
22, name=3D61-dev-root-link.rules
[  761.608521] __ratelimit: 210 callbacks suppressed

Signed-off-by: Wu Fengguang <fengguang.wu@intel.com>
---
 fs/notify/inotify/inotify_user.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

--- linux.orig/fs/notify/inotify/inotify_user.c
+++ linux/fs/notify/inotify/inotify_user.c
@@ -318,7 +318,9 @@ static long inotify_ioctl(struct file *f
 		mutex_lock(&group->notification_mutex);
 		list_for_each_entry(holder, &group->notification_list, event_list) {
 			event =3D holder->event;
-			send_len +=3D sizeof(struct inotify_event) + event->name_len;
+			send_len +=3D sizeof(struct inotify_event);
+			send_len +=3D roundup(event->name_len,
+					sizeof(struct inotify_event));
 		}
 		mutex_unlock(&group->notification_mutex);
 		ret =3D put_user(send_len, (int __user *) p);
--
To unsubscribe from this list: send the line "unsubscribe linux-hotplug=
" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

  reply	other threads:[~2009-05-07 11:59 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2009-05-03 10:54 very slow NFS boot on linux-next and -mm Wu Fengguang
2009-05-03 12:40 ` Wu Fengguang
2009-05-03 12:40   ` Wu Fengguang
2009-05-03 12:56   ` Kay Sievers
2009-05-03 12:56     ` Kay Sievers
     [not found]     ` <20090504133019.GA14300@localhost>
2009-05-06 13:42       ` Kay Sievers
2009-05-06 13:42         ` Kay Sievers
2009-05-07 11:59         ` Wu Fengguang [this message]
2009-05-07 11:59           ` [PATCH] inotify: report rounded-up event size to user space Wu Fengguang
2009-05-07 12:16           ` Eric Paris
2009-05-07 12:16             ` Eric Paris

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20090507115947.GA20934@localhost \
    --to=fengguang.wu@intel.com \
    --cc=Trond.Myklebust@netapp.com \
    --cc=akpm@linux-foundation.org \
    --cc=eparis@redhat.com \
    --cc=hch@lst.de \
    --cc=kay.sievers@vrfy.org \
    --cc=linux-hotplug@vger.kernel.org \
    --cc=linux-nfs@vger.kernel.org \
    --cc=viro@zeniv.linux.org.uk \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.