more POLL... fun

more POLL... fun

[Al Viro]

On cross-builds the __poll_t annotations had caught something interesting:
void spufs_mfc_callback(struct spu *spu)
{
	....
                mask = 0;
                if (free_elements & 0xffff)
                        mask |= POLLOUT;
                if (tagstatus & ctx->tagwait)
                        mask |= POLLIN;

                kill_fasync(&ctx->mfc_fasync, SIGIO, mask);
	....
}

That's arch/powerpc/platforms/cell/spufs/file.c.  WTF is kill_fasync()
getting as the last argument here?  Valid values are
#define POLL_IN         (__SI_POLL|1)   /* data input available */
#define POLL_OUT        (__SI_POLL|2)   /* output buffers available */
#define POLL_MSG        (__SI_POLL|3)   /* input message available */
#define POLL_ERR        (__SI_POLL|4)   /* i/o error */
#define POLL_PRI        (__SI_POLL|5)   /* high priority input available */
#define POLL_HUP        (__SI_POLL|6)   /* device disconnected */

Use of POLLIN, POLLOUT, etc. here is wrong - kill_fasync() will step into
                        BUG_ON((reason & __SI_MASK) != __SI_POLL);
in send_sigio_to_task().  Other two callers of kill_fasync() in that file
are trivially fixed by switching to POLL_IN and POLL_OUT; with this one
I've no idea what had been intended.

What's more, I really wonder if it had _ever_ been tested - these kill_fasync()
calls had been introduced in
commit 8b3d6663c6217e4f50cc3720935a96da9b984117
Author: Arnd Bergmann <arnd@arndb.de>
Date:   Tue Nov 15 15:53:52 2005 -0500

    [PATCH] spufs: cooperative scheduler support
more than 5 years after that BUG_ON() had been added - it goes back to
+                       /* Make sure we are called with one of the POLL_*
+                          reasons, otherwise we could leak kernel stack into
+                          userspace.  */
+                       if ((reason & __SI_MASK) != __SI_POLL)
+                               BUG();
in 2.3.99pre-10-3, on May 25 2000.

What the hell am I missing here?  Has that code been DOA and never used by
anyone in all the decade it had been in mainline?

Re: more POLL... fun

[Arnd Bergmann]

On Friday 04 December 2015 06:38:25 Al Viro wrote:
> On cross-builds the __poll_t annotations had caught something interesting:
> void spufs_mfc_callback(struct spu *spu)
> {
> 	....
>                 mask = 0;
>                 if (free_elements & 0xffff)
>                         mask |= POLLOUT;
>                 if (tagstatus & ctx->tagwait)
>                         mask |= POLLIN;
> 
>                 kill_fasync(&ctx->mfc_fasync, SIGIO, mask);
> 	....
> }
> 
> That's arch/powerpc/platforms/cell/spufs/file.c.  WTF is kill_fasync()
> getting as the last argument here?  Valid values are
> #define POLL_IN         (__SI_POLL|1)   /* data input available */
> #define POLL_OUT        (__SI_POLL|2)   /* output buffers available */
> #define POLL_MSG        (__SI_POLL|3)   /* input message available */
> #define POLL_ERR        (__SI_POLL|4)   /* i/o error */
> #define POLL_PRI        (__SI_POLL|5)   /* high priority input available */
> #define POLL_HUP        (__SI_POLL|6)   /* device disconnected */
> 
> Use of POLLIN, POLLOUT, etc. here is wrong - kill_fasync() will step into
>                         BUG_ON((reason & __SI_MASK) != __SI_POLL);
> in send_sigio_to_task().  Other two callers of kill_fasync() in that file
> are trivially fixed by switching to POLL_IN and POLL_OUT; with this one
> I've no idea what had been intended.
> 
> What's more, I really wonder if it had _ever_ been tested - these kill_fasync()
> calls had been introduced in
> commit 8b3d6663c6217e4f50cc3720935a96da9b984117
> Author: Arnd Bergmann <arnd@arndb.de>
> Date:   Tue Nov 15 15:53:52 2005 -0500
> 
>     [PATCH] spufs: cooperative scheduler support
> more than 5 years after that BUG_ON() had been added - it goes back to
> +                       /* Make sure we are called with one of the POLL_*
> +                          reasons, otherwise we could leak kernel stack into
> +                          userspace.  */
> +                       if ((reason & __SI_MASK) != __SI_POLL)
> +                               BUG();
> in 2.3.99pre-10-3, on May 25 2000.
> 
> What the hell am I missing here?  Has that code been DOA and never used by
> anyone in all the decade it had been in mainline?

I don't remember why we put in fasync support, but I have checked the libspe
implementation and found that it doesn't use it (not a big surprise there).
It always uses epoll() to get notifications from spufs, and based on your
explanation I assume everything else (there may have been one or two users
that used the low-level interfaces rather than libspe) did too.

	Arnd

Re: more POLL... fun

[Al Viro]

On Fri, Dec 04, 2015 at 10:16:50AM +0100, Arnd Bergmann wrote:

> I don't remember why we put in fasync support, but I have checked the libspe
> implementation and found that it doesn't use it (not a big surprise there).
> It always uses epoll() to get notifications from spufs, and based on your
> explanation I assume everything else (there may have been one or two users
> that used the low-level interfaces rather than libspe) did too.

OK...  So should we just rip ->{mfc,ibox,wbox}_fasync out, along with all
three kill_fasync() and ->fasync() instances in there?  We obviously need to
leave spufs_{mfc,ibox,wbox}_callback() in place for the sake of those
wake_up_all(&ctx->{mfc,ibox,wbox}_wq); in them...

I mean, fasync in there obviously never been used at all - it never delivered
a single SIGIO, and the first user to try would get the BUG_ON() in fcntl.c
instead.  Since nobody complained in more than 10 years...

Re: more POLL... fun

[Arnd Bergmann]

On Friday 04 December 2015 15:21:33 Al Viro wrote:
> On Fri, Dec 04, 2015 at 10:16:50AM +0100, Arnd Bergmann wrote:
> 
> > I don't remember why we put in fasync support, but I have checked the libspe
> > implementation and found that it doesn't use it (not a big surprise there).
> > It always uses epoll() to get notifications from spufs, and based on your
> > explanation I assume everything else (there may have been one or two users
> > that used the low-level interfaces rather than libspe) did too.
> 
> OK...  So should we just rip ->{mfc,ibox,wbox}_fasync out, along with all
> three kill_fasync() and ->fasync() instances in there?  We obviously need to
> leave spufs_{mfc,ibox,wbox}_callback() in place for the sake of those
> wake_up_all(&ctx->{mfc,ibox,wbox}_wq); in them...
> 
> I mean, fasync in there obviously never been used at all - it never delivered
> a single SIGIO, and the first user to try would get the BUG_ON() in fcntl.c
> instead.  Since nobody complained in more than 10 years...

Yes, I think that would be best. Do you want me to send that patch, or do
you prefer to do it yourself? In theory that patch should also go into stable
kernels, but I suspect nobody who still owns a machine that is able to run
this code will ever upgrade to a stable release, so we probably don't need
that.

	Arnd

Re: more POLL... fun

[Al Viro]

On Fri, Dec 04, 2015 at 06:13:54PM +0100, Arnd Bergmann wrote:

> Yes, I think that would be best. Do you want me to send that patch, or do
> you prefer to do it yourself? In theory that patch should also go into stable
> kernels, but I suspect nobody who still owns a machine that is able to run
> this code will ever upgrade to a stable release, so we probably don't need
> that.

Probably better if it goes through ppc tree - the only relationship it has
to VFS is broken calls of kill_fasync() in now-removed code...  Something
like this, perhaps?

[spufs] get rid of broken fasync stuff
 
In all the years it's been in the tree it had never been used by
anything - it would instantly trigger BUG_ON() in fs/fcntl.c due
to bogus band argument passed to kill_fasync().  Since nobody
had ever used it in ten years, let's just rip it out and be
done with that.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---

diff --git a/arch/powerpc/platforms/cell/spufs/file.c b/arch/powerpc/platforms/cell/spufs/file.c
index 5038fd5..88b7613 100644
--- a/arch/powerpc/platforms/cell/spufs/file.c
+++ b/arch/powerpc/platforms/cell/spufs/file.c
@@ -686,23 +686,13 @@ size_t spu_ibox_read(struct spu_context *ctx, u32 *data)
 	return ctx->ops->ibox_read(ctx, data);
 }
 
-static int spufs_ibox_fasync(int fd, struct file *file, int on)
-{
-	struct spu_context *ctx = file->private_data;
-
-	return fasync_helper(fd, file, on, &ctx->ibox_fasync);
-}
-
 /* interrupt-level ibox callback function. */
 void spufs_ibox_callback(struct spu *spu)
 {
 	struct spu_context *ctx = spu->ctx;
 
-	if (!ctx)
-		return;
-
-	wake_up_all(&ctx->ibox_wq);
-	kill_fasync(&ctx->ibox_fasync, SIGIO, POLLIN);
+	if (ctx)
+		wake_up_all(&ctx->ibox_wq);
 }
 
 /*
@@ -797,7 +787,6 @@ static const struct file_operations spufs_ibox_fops = {
 	.open	= spufs_pipe_open,
 	.read	= spufs_ibox_read,
 	.poll	= spufs_ibox_poll,
-	.fasync	= spufs_ibox_fasync,
 	.llseek = no_llseek,
 };
 
@@ -835,26 +824,13 @@ size_t spu_wbox_write(struct spu_context *ctx, u32 data)
 	return ctx->ops->wbox_write(ctx, data);
 }
 
-static int spufs_wbox_fasync(int fd, struct file *file, int on)
-{
-	struct spu_context *ctx = file->private_data;
-	int ret;
-
-	ret = fasync_helper(fd, file, on, &ctx->wbox_fasync);
-
-	return ret;
-}
-
 /* interrupt-level wbox callback function. */
 void spufs_wbox_callback(struct spu *spu)
 {
 	struct spu_context *ctx = spu->ctx;
 
-	if (!ctx)
-		return;
-
-	wake_up_all(&ctx->wbox_wq);
-	kill_fasync(&ctx->wbox_fasync, SIGIO, POLLOUT);
+	if (ctx)
+		wake_up_all(&ctx->wbox_wq);
 }
 
 /*
@@ -947,7 +923,6 @@ static const struct file_operations spufs_wbox_fops = {
 	.open	= spufs_pipe_open,
 	.write	= spufs_wbox_write,
 	.poll	= spufs_wbox_poll,
-	.fasync	= spufs_wbox_fasync,
 	.llseek = no_llseek,
 };
 
@@ -1523,28 +1498,8 @@ void spufs_mfc_callback(struct spu *spu)
 {
 	struct spu_context *ctx = spu->ctx;
 
-	if (!ctx)
-		return;
-
-	wake_up_all(&ctx->mfc_wq);
-
-	pr_debug("%s %s\n", __func__, spu->name);
-	if (ctx->mfc_fasync) {
-		u32 free_elements, tagstatus;
-		unsigned int mask;
-
-		/* no need for spu_acquire in interrupt context */
-		free_elements = ctx->ops->get_mfc_free_elements(ctx);
-		tagstatus = ctx->ops->read_mfc_tagstatus(ctx);
-
-		mask = 0;
-		if (free_elements & 0xffff)
-			mask |= POLLOUT;
-		if (tagstatus & ctx->tagwait)
-			mask |= POLLIN;
-
-		kill_fasync(&ctx->mfc_fasync, SIGIO, mask);
-	}
+	if (ctx)
+		wake_up_all(&ctx->mfc_wq);
 }
 
 static int spufs_read_mfc_tagstatus(struct spu_context *ctx, u32 *status)
@@ -1806,13 +1761,6 @@ static int spufs_mfc_fsync(struct file *file, loff_t start, loff_t end, int data
 	return err;
 }
 
-static int spufs_mfc_fasync(int fd, struct file *file, int on)
-{
-	struct spu_context *ctx = file->private_data;
-
-	return fasync_helper(fd, file, on, &ctx->mfc_fasync);
-}
-
 static const struct file_operations spufs_mfc_fops = {
 	.open	 = spufs_mfc_open,
 	.release = spufs_mfc_release,
@@ -1821,7 +1769,6 @@ static const struct file_operations spufs_mfc_fops = {
 	.poll	 = spufs_mfc_poll,
 	.flush	 = spufs_mfc_flush,
 	.fsync	 = spufs_mfc_fsync,
-	.fasync	 = spufs_mfc_fasync,
 	.mmap	 = spufs_mfc_mmap,
 	.llseek  = no_llseek,
 };
diff --git a/arch/powerpc/platforms/cell/spufs/spufs.h b/arch/powerpc/platforms/cell/spufs/spufs.h
index bcfd6f0..aac7339 100644
--- a/arch/powerpc/platforms/cell/spufs/spufs.h
+++ b/arch/powerpc/platforms/cell/spufs/spufs.h
@@ -102,9 +102,6 @@ struct spu_context {
 	wait_queue_head_t stop_wq;
 	wait_queue_head_t mfc_wq;
 	wait_queue_head_t run_wq;
-	struct fasync_struct *ibox_fasync;
-	struct fasync_struct *wbox_fasync;
-	struct fasync_struct *mfc_fasync;
 	u32 tagwait;
 	struct spu_context_ops *ops;
 	struct work_struct reap_work;

Re: more POLL... fun

[Arnd Bergmann]

On Friday 04 December 2015 17:30:31 Al Viro wrote:
> On Fri, Dec 04, 2015 at 06:13:54PM +0100, Arnd Bergmann wrote:
> 
> > Yes, I think that would be best. Do you want me to send that patch, or do
> > you prefer to do it yourself? In theory that patch should also go into stable
> > kernels, but I suspect nobody who still owns a machine that is able to run
> > this code will ever upgrade to a stable release, so we probably don't need
> > that.
> 
> Probably better if it goes through ppc tree - the only relationship it has
> to VFS is broken calls of kill_fasync() in now-removed code...

Right.

>  Something like this, perhaps?
>
> [spufs] get rid of broken fasync stuff
>  
> In all the years it's been in the tree it had never been used by
> anything - it would instantly trigger BUG_ON() in fs/fcntl.c due
> to bogus band argument passed to kill_fasync().  Since nobody
> had ever used it in ten years, let's just rip it out and be
> done with that.
> 
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---

Looks all good, thanks!

Acked-by: Arnd Bergmann <arnd@arndb.de>

Re: more POLL... fun

[Michael Ellerman]

On Fri, 2015-12-04 at 17:30 +0000, Al Viro wrote:
> On Fri, Dec 04, 2015 at 06:13:54PM +0100, Arnd Bergmann wrote:
> > Yes, I think that would be best. Do you want me to send that patch, or do
> > you prefer to do it yourself? In theory that patch should also go into stable
> > kernels, but I suspect nobody who still owns a machine that is able to run
> > this code will ever upgrade to a stable release, so we probably don't need
> > that.
> 
> Probably better if it goes through ppc tree - the only relationship it has
> to VFS is broken calls of kill_fasync() in now-removed code...  Something
> like this, perhaps?

Yeah that looks fine to me. I'll take it into powerpc#next for 4.5.

cheers

Re: more POLL... fun

[Michael Ellerman]

On Fri, 2015-12-04 at 17:30:31 UTC, Al Viro wrote:
> On Fri, Dec 04, 2015 at 06:13:54PM +0100, Arnd Bergmann wrote:
> 
> > Yes, I think that would be best. Do you want me to send that patch, or do
> > you prefer to do it yourself? In theory that patch should also go into stable
> > kernels, but I suspect nobody who still owns a machine that is able to run
> > this code will ever upgrade to a stable release, so we probably don't need
> > that.
> 
> Probably better if it goes through ppc tree - the only relationship it has
> to VFS is broken calls of kill_fasync() in now-removed code...  Something
> like this, perhaps?
> 
> [spufs] get rid of broken fasync stuff
>  
> In all the years it's been in the tree it had never been used by
> anything - it would instantly trigger BUG_ON() in fs/fcntl.c due
> to bogus band argument passed to kill_fasync().  Since nobody
> had ever used it in ten years, let's just rip it out and be
> done with that.
> 
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> Acked-by: Arnd Bergmann <arnd@arndb.de>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/7d7be3aa08fa338e84eb235805ee18

cheers