[PATCH v2] pasta, passt-repair: Support multiple events per read() in inotify handlers

[PATCH v2] pasta, passt-repair: Support multiple events per read() in inotify handlers

[David Gibson]

From: Stefano Brivio <sbrivio@redhat.com>

The current code assumes that we'll get one event per read() on
inotify descriptors, but that's not the case, not from documentation,
and not from reports.

Add loops in the two inotify handlers we have, in pasta-specific code
and passt-repair, to go through all the events we receive.

Link: https://bugs.passt.top/show_bug.cgi?id=119

Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
[dwg: Remove unnecessary buffer expansion, use strnlen instead of strlen
 to make Coverity happier]
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 passt-repair.c | 27 ++++++++++++++++++++-------
 pasta.c        | 20 +++++++++++++-------
 2 files changed, 33 insertions(+), 14 deletions(-)

diff --git a/passt-repair.c b/passt-repair.c
index 120f7aa7..d36c9c94 100644
--- a/passt-repair.c
+++ b/passt-repair.c
@@ -111,14 +111,14 @@ int main(int argc, char **argv)
 	}
 
 	if ((sb.st_mode & S_IFMT) == S_IFDIR) {
-		char buf[sizeof(struct inotify_event) + NAME_MAX + 1];
+		char buf[sizeof(struct inotify_event) + NAME_MAX + 1]
+		   __attribute__ ((aligned(__alignof__(struct inotify_event))));
 		const struct inotify_event *ev;
 		char path[PATH_MAX + 1];
+		bool found = false;
 		ssize_t n;
 		int fd;
 
-		ev = (struct inotify_event *)buf;
-
 		if ((fd = inotify_init1(IN_CLOEXEC)) < 0) {
 			fprintf(stderr, "inotify_init1: %i\n", errno);
 			_exit(1);
@@ -130,6 +130,8 @@ int main(int argc, char **argv)
 		}
 
 		do {
+			char *p;
+
 			n = read(fd, buf, sizeof(buf));
 			if (n < 0) {
 				fprintf(stderr, "inotify read: %i", errno);
@@ -138,11 +140,22 @@ int main(int argc, char **argv)
 
 			if (n < (ssize_t)sizeof(*ev)) {
 				fprintf(stderr, "Short inotify read: %zi", n);
-				_exit(1);
+				continue;
+			}
+
+			for (p = buf; p < buf + n; p += sizeof(*ev) + ev->len) {
+				ev = (const struct inotify_event *)p;
+
+				if (ev->len >= REPAIR_EXT_LEN &&
+				    !memcmp(ev->name +
+					    strnlen(ev->name, ev->len) -
+					    REPAIR_EXT_LEN,
+					    REPAIR_EXT, REPAIR_EXT_LEN)) {
+					found = true;
+					break;
+				}
 			}
-		} while (ev->len < REPAIR_EXT_LEN ||
-			 memcmp(ev->name + strlen(ev->name) - REPAIR_EXT_LEN,
-				REPAIR_EXT, REPAIR_EXT_LEN));
+		} while (!found);
 
 		snprintf(path, sizeof(path), "%s/%s", argv[1], ev->name);
 		if ((stat(path, &sb))) {
diff --git a/pasta.c b/pasta.c
index fa3e7de5..017fa322 100644
--- a/pasta.c
+++ b/pasta.c
@@ -498,17 +498,23 @@ void pasta_netns_quit_init(const struct ctx *c)
  */
 void pasta_netns_quit_inotify_handler(struct ctx *c, int inotify_fd)
 {
-	char buf[sizeof(struct inotify_event) + NAME_MAX + 1];
-	const struct inotify_event *in_ev = (struct inotify_event *)buf;
+	char buf[sizeof(struct inotify_event) + NAME_MAX + 1]
+		__attribute__ ((aligned(__alignof__(struct inotify_event))));
+	const struct inotify_event *ev;
+	ssize_t n;
+	char *p;
 
-	if (read(inotify_fd, buf, sizeof(buf)) < (ssize_t)sizeof(*in_ev))
+	if ((n = read(inotify_fd, buf, sizeof(buf))) < (ssize_t)sizeof(*ev))
 		return;
 
-	if (strncmp(in_ev->name, c->netns_base, sizeof(c->netns_base)))
-		return;
+	for (p = buf; p < buf + n; p += sizeof(*ev) + ev->len) {
+		ev = (const struct inotify_event *)p;
 
-	info("Namespace %s is gone, exiting", c->netns_base);
-	_exit(EXIT_SUCCESS);
+		if (!strncmp(ev->name, c->netns_base, sizeof(c->netns_base))) {
+			info("Namespace %s is gone, exiting", c->netns_base);
+			_exit(EXIT_SUCCESS);
+		}
+	}
 }
 
 /**
-- 
@@ -498,17 +498,23 @@ void pasta_netns_quit_init(const struct ctx *c)
  */
 void pasta_netns_quit_inotify_handler(struct ctx *c, int inotify_fd)
 {
-	char buf[sizeof(struct inotify_event) + NAME_MAX + 1];
-	const struct inotify_event *in_ev = (struct inotify_event *)buf;
+	char buf[sizeof(struct inotify_event) + NAME_MAX + 1]
+		__attribute__ ((aligned(__alignof__(struct inotify_event))));
+	const struct inotify_event *ev;
+	ssize_t n;
+	char *p;
 
-	if (read(inotify_fd, buf, sizeof(buf)) < (ssize_t)sizeof(*in_ev))
+	if ((n = read(inotify_fd, buf, sizeof(buf))) < (ssize_t)sizeof(*ev))
 		return;
 
-	if (strncmp(in_ev->name, c->netns_base, sizeof(c->netns_base)))
-		return;
+	for (p = buf; p < buf + n; p += sizeof(*ev) + ev->len) {
+		ev = (const struct inotify_event *)p;
 
-	info("Namespace %s is gone, exiting", c->netns_base);
-	_exit(EXIT_SUCCESS);
+		if (!strncmp(ev->name, c->netns_base, sizeof(c->netns_base))) {
+			info("Namespace %s is gone, exiting", c->netns_base);
+			_exit(EXIT_SUCCESS);
+		}
+	}
 }
 
 /**
-- 
2.49.0

Re: [PATCH v2] pasta, passt-repair: Support multiple events per read() in inotify handlers

[Stefano Brivio]

On Fri, 28 Mar 2025 11:39:58 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> From: Stefano Brivio <sbrivio@redhat.com>
> 
> The current code assumes that we'll get one event per read() on
> inotify descriptors, but that's not the case, not from documentation,
> and not from reports.
> 
> Add loops in the two inotify handlers we have, in pasta-specific code
> and passt-repair, to go through all the events we receive.
> 
> Link: https://bugs.passt.top/show_bug.cgi?id=119
> 
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> [dwg: Remove unnecessary buffer expansion, use strnlen instead of strlen
>  to make Coverity happier]

I have to admit I didn't know about strnlen(). POSIX.1-2008, that's
bleeding edge stuff. :)

That wasn't actually enough though: in my Coverity scan, it was also
complaining about:

> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  passt-repair.c | 27 ++++++++++++++++++++-------
>  pasta.c        | 20 +++++++++++++-------
>  2 files changed, 33 insertions(+), 14 deletions(-)
> 
> diff --git a/passt-repair.c b/passt-repair.c
> index 120f7aa7..d36c9c94 100644
> --- a/passt-repair.c
> +++ b/passt-repair.c
> @@ -111,14 +111,14 @@ int main(int argc, char **argv)
>  	}
>  
>  	if ((sb.st_mode & S_IFMT) == S_IFDIR) {
> -		char buf[sizeof(struct inotify_event) + NAME_MAX + 1];
> +		char buf[sizeof(struct inotify_event) + NAME_MAX + 1]
> +		   __attribute__ ((aligned(__alignof__(struct inotify_event))));
>  		const struct inotify_event *ev;
>  		char path[PATH_MAX + 1];
> +		bool found = false;
>  		ssize_t n;
>  		int fd;
>  
> -		ev = (struct inotify_event *)buf;
> -
>  		if ((fd = inotify_init1(IN_CLOEXEC)) < 0) {
>  			fprintf(stderr, "inotify_init1: %i\n", errno);
>  			_exit(1);
> @@ -130,6 +130,8 @@ int main(int argc, char **argv)
>  		}
>  
>  		do {
> +			char *p;
> +
>  			n = read(fd, buf, sizeof(buf));
>  			if (n < 0) {
>  				fprintf(stderr, "inotify read: %i", errno);
> @@ -138,11 +140,22 @@ int main(int argc, char **argv)
>  
>  			if (n < (ssize_t)sizeof(*ev)) {
>  				fprintf(stderr, "Short inotify read: %zi", n);
> -				_exit(1);
> +				continue;
> +			}
> +
> +			for (p = buf; p < buf + n; p += sizeof(*ev) + ev->len) {
> +				ev = (const struct inotify_event *)p;
> +
> +				if (ev->len >= REPAIR_EXT_LEN &&
> +				    !memcmp(ev->name +
> +					    strnlen(ev->name, ev->len) -
> +					    REPAIR_EXT_LEN,
> +					    REPAIR_EXT, REPAIR_EXT_LEN)) {
> +					found = true;
> +					break;
> +				}
>  			}
> -		} while (ev->len < REPAIR_EXT_LEN ||
> -			 memcmp(ev->name + strlen(ev->name) - REPAIR_EXT_LEN,
> -				REPAIR_EXT, REPAIR_EXT_LEN));
> +		} while (!found);
>  
>  		snprintf(path, sizeof(path), "%s/%s", argv[1], ev->name);

...ev->name not being necessarily NULL-terminated and passed to
snprintf() here. So I applied this with an additional (and useless, unless
the kernel is unrealistically buggy):

		if (ev->len > NAME_MAX + 1 || ev->name[ev->len] != '\0') {
			fprintf(stderr, "Invalid filename from inotify\n");
			_exit(1);
		}

-- 
Stefano

Re: [PATCH v2] pasta, passt-repair: Support multiple events per read() in inotify handlers

[David Gibson]

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

On Fri, Mar 28, 2025 at 03:29:10PM +0100, Stefano Brivio wrote:
> On Fri, 28 Mar 2025 11:39:58 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > From: Stefano Brivio <sbrivio@redhat.com>
> > 
> > The current code assumes that we'll get one event per read() on
> > inotify descriptors, but that's not the case, not from documentation,
> > and not from reports.
> > 
> > Add loops in the two inotify handlers we have, in pasta-specific code
> > and passt-repair, to go through all the events we receive.
> > 
> > Link: https://bugs.passt.top/show_bug.cgi?id=119
> > 
> > Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> > [dwg: Remove unnecessary buffer expansion, use strnlen instead of strlen
> >  to make Coverity happier]
> 
> I have to admit I didn't know about strnlen(). POSIX.1-2008, that's
> bleeding edge stuff. :)
> 
> That wasn't actually enough though: in my Coverity scan, it was also
> complaining about:
> 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  passt-repair.c | 27 ++++++++++++++++++++-------
> >  pasta.c        | 20 +++++++++++++-------
> >  2 files changed, 33 insertions(+), 14 deletions(-)
> > 
> > diff --git a/passt-repair.c b/passt-repair.c
> > index 120f7aa7..d36c9c94 100644
> > --- a/passt-repair.c
> > +++ b/passt-repair.c
> > @@ -111,14 +111,14 @@ int main(int argc, char **argv)
> >  	}
> >  
> >  	if ((sb.st_mode & S_IFMT) == S_IFDIR) {
> > -		char buf[sizeof(struct inotify_event) + NAME_MAX + 1];
> > +		char buf[sizeof(struct inotify_event) + NAME_MAX + 1]
> > +		   __attribute__ ((aligned(__alignof__(struct inotify_event))));
> >  		const struct inotify_event *ev;
> >  		char path[PATH_MAX + 1];
> > +		bool found = false;
> >  		ssize_t n;
> >  		int fd;
> >  
> > -		ev = (struct inotify_event *)buf;
> > -
> >  		if ((fd = inotify_init1(IN_CLOEXEC)) < 0) {
> >  			fprintf(stderr, "inotify_init1: %i\n", errno);
> >  			_exit(1);
> > @@ -130,6 +130,8 @@ int main(int argc, char **argv)
> >  		}
> >  
> >  		do {
> > +			char *p;
> > +
> >  			n = read(fd, buf, sizeof(buf));
> >  			if (n < 0) {
> >  				fprintf(stderr, "inotify read: %i", errno);
> > @@ -138,11 +140,22 @@ int main(int argc, char **argv)
> >  
> >  			if (n < (ssize_t)sizeof(*ev)) {
> >  				fprintf(stderr, "Short inotify read: %zi", n);
> > -				_exit(1);
> > +				continue;
> > +			}
> > +
> > +			for (p = buf; p < buf + n; p += sizeof(*ev) + ev->len) {
> > +				ev = (const struct inotify_event *)p;
> > +
> > +				if (ev->len >= REPAIR_EXT_LEN &&
> > +				    !memcmp(ev->name +
> > +					    strnlen(ev->name, ev->len) -
> > +					    REPAIR_EXT_LEN,
> > +					    REPAIR_EXT, REPAIR_EXT_LEN)) {
> > +					found = true;
> > +					break;
> > +				}
> >  			}
> > -		} while (ev->len < REPAIR_EXT_LEN ||
> > -			 memcmp(ev->name + strlen(ev->name) - REPAIR_EXT_LEN,
> > -				REPAIR_EXT, REPAIR_EXT_LEN));
> > +		} while (!found);
> >  
> >  		snprintf(path, sizeof(path), "%s/%s", argv[1], ev->name);
> 
> ...ev->name not being necessarily NULL-terminated and passed to
> snprintf() here. So I applied this with an additional (and useless, unless
> the kernel is unrealistically buggy):

Ah, yeah, I saw that error but wasn't sure how to fix it.

> 		if (ev->len > NAME_MAX + 1 || ev->name[ev->len] != '\0') {
> 			fprintf(stderr, "Invalid filename from inotify\n");
> 			_exit(1);
> 		}

That should do it.

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]