Re: [GIT PULL] VFIO fixes for v4.1-rc2
On Fri, 2015-05-01 at 13:23 -0700, Linus Torvalds wrote: > On Fri, May 1, 2015 at 11:48 AM, Alex Williamson > wrote: > > > > Ok. It seemed like useful behavior to be able to provide some response > > to the user in the event that a ->remove handler is blocked by a device > > in-use and the user attempts to abort the action. > > Well, that kind of notification *might* be useful, but at the cost of > saying "somebody tried to send you a signal, so I am now telling you > about it, and then deleting that signal, and you'll never know what it > actually was"? > > That's not useful, that's just wrong. Yep, it was a bad idea. > Now, what might in theory be useful - but I haven't actually seen > anybody do anything like that - is to start out with an interruptible > sleep, warn if you get interrupted, and then continue with an > un-interruptible sleep (leaving the signal active). I was considering doing exactly this. > But even that sounds like a very special case, and I don't think > anything has ever done that. > > In general, our signal handling falls into three distinct categories: > > (a) interruptible (and you can cancel the operation and return "try again") > > (b) killable (you can cancel the operation, knowing that the > requester will be killed and won't try again) > > (c) uninterruptible > > where that (b) tends to be a special case of an operation that > technically isn't really interruptible (because the ABI doesn't allow > for retrying or error returns), but knowing that the caller will never > see the error case because it's killed means that you can do it. The > classic example of that is an NFS mount that is mounted "nointr" - you > can't return EINTR for a read or a write (because that invalidates > POSIX) but you want to let SIGKILL just kill the process in the middle > when the network is hung. I think we're in that (c) case unless we want to change our driver API to allow driver remove callbacks to return error. Killing the caller doesn't really help the situation without being able to back out of the remove path. Killing the task with the device open would help, but seems rather harsh. I expect we eventually want to be able to escalate to revoking the device from the user, but currently we only have a notifier to request the device from cooperative users. In the event of an uncooperative user, we block, which can be difficult to figure out, especially when we're dealing with SR-IOV devices and a PF unbind implicitly induces a VF unbind. The interruptible component here is simply a logging mechanism which should have turned into an "interruptible_once" rather than a signal flush. I try to avoid vfio being a special case, but maybe in this instance it's worthwhile. If you have other suggestions, please let me know. Thanks, Alex -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [GIT PULL] VFIO fixes for v4.1-rc2
...of course I meant t-> and not current->
On 5/1/15, Richard Weinberger wrote:
> On Fri, May 1, 2015 at 8:37 PM, Linus Torvalds
> wrote:
>> "flush_signals()" is only for kernel threads, where it's a hacky
>> alternative to actually handling them (since kernel threads never
>> rreturn to user space and cannot really "handle" a signal). But you're
>> doing it in the ->remove handler for the device, which can be called
>> by arbitrary system processes. This is not a kernel thread thing, as
>> far as I can see.
>>
>> If you cannot handle signals, you damn well shouldn't be using
>> "wait_event_interruptible_timeout()" to begin with. Get rid of the
>> "interruptible", since it apparently *isn't* interruptible.
>>
>> So I'm not pulling this.
>>
>> Now I'm worried that other drivers do insane things like this. I
>> wonder if we should add some sanity test to flush_signals() to make
>> sure that it can only ever get called from a kernel thread.
>
> Hmm, a quick grep exposes some questionable users.
> At least w1 looks fishy.
> drivers/w1/w1_family.c:w1_unregister_family
> drivers/w1/w1_int.c:__w1_remove_master_device
>
> What do you think about a WARN_ON like:
>
> diff --git a/kernel/signal.c b/kernel/signal.c
> index d51c5dd..b4079c3 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -427,6 +427,8 @@ void flush_signals(struct task_struct *t)
> {
> unsigned long flags;
>
> + WARN_ON((current->flags & PF_KTHREAD) == 0);
> +
> spin_lock_irqsave(>sighand->siglock, flags);
> __flush_signals(t);
> spin_unlock_irqrestore(>sighand->siglock, flags);
>
> --
> Thanks,
> //richard
>
--
Thanks,
//richard
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [GIT PULL] VFIO fixes for v4.1-rc2
On Fri, May 1, 2015 at 11:48 AM, Alex Williamson wrote: > > Ok. It seemed like useful behavior to be able to provide some response > to the user in the event that a ->remove handler is blocked by a device > in-use and the user attempts to abort the action. Well, that kind of notification *might* be useful, but at the cost of saying "somebody tried to send you a signal, so I am now telling you about it, and then deleting that signal, and you'll never know what it actually was"? That's not useful, that's just wrong. Now, what might in theory be useful - but I haven't actually seen anybody do anything like that - is to start out with an interruptible sleep, warn if you get interrupted, and then continue with an un-interruptible sleep (leaving the signal active). But even that sounds like a very special case, and I don't think anything has ever done that. In general, our signal handling falls into three distinct categories: (a) interruptible (and you can cancel the operation and return "try again") (b) killable (you can cancel the operation, knowing that the requester will be killed and won't try again) (c) uninterruptible where that (b) tends to be a special case of an operation that technically isn't really interruptible (because the ABI doesn't allow for retrying or error returns), but knowing that the caller will never see the error case because it's killed means that you can do it. The classic example of that is an NFS mount that is mounted "nointr" - you can't return EINTR for a read or a write (because that invalidates POSIX) but you want to let SIGKILL just kill the process in the middle when the network is hung. Linus -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [GIT PULL] VFIO fixes for v4.1-rc2
On Fri, May 1, 2015 at 8:37 PM, Linus Torvalds
wrote:
> "flush_signals()" is only for kernel threads, where it's a hacky
> alternative to actually handling them (since kernel threads never
> rreturn to user space and cannot really "handle" a signal). But you're
> doing it in the ->remove handler for the device, which can be called
> by arbitrary system processes. This is not a kernel thread thing, as
> far as I can see.
>
> If you cannot handle signals, you damn well shouldn't be using
> "wait_event_interruptible_timeout()" to begin with. Get rid of the
> "interruptible", since it apparently *isn't* interruptible.
>
> So I'm not pulling this.
>
> Now I'm worried that other drivers do insane things like this. I
> wonder if we should add some sanity test to flush_signals() to make
> sure that it can only ever get called from a kernel thread.
Hmm, a quick grep exposes some questionable users.
At least w1 looks fishy.
drivers/w1/w1_family.c:w1_unregister_family
drivers/w1/w1_int.c:__w1_remove_master_device
What do you think about a WARN_ON like:
diff --git a/kernel/signal.c b/kernel/signal.c
index d51c5dd..b4079c3 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -427,6 +427,8 @@ void flush_signals(struct task_struct *t)
{
unsigned long flags;
+ WARN_ON((current->flags & PF_KTHREAD) == 0);
+
spin_lock_irqsave(>sighand->siglock, flags);
__flush_signals(t);
spin_unlock_irqrestore(>sighand->siglock, flags);
--
Thanks,
//richard
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [GIT PULL] VFIO fixes for v4.1-rc2
On Fri, 2015-05-01 at 11:37 -0700, Linus Torvalds wrote: > On Fri, May 1, 2015 at 10:40 AM, Alex Williamson > wrote: > > > > - Flush signals on interrupted wait to retain polling interval (Alex > > Williamson) > > This cannot *possibly* be right. If I read this patch right, you're > randomly just getting rid of signals. No way in hell is that correct. > > "flush_signals()" is only for kernel threads, where it's a hacky > alternative to actually handling them (since kernel threads never > rreturn to user space and cannot really "handle" a signal). But you're > doing it in the ->remove handler for the device, which can be called > by arbitrary system processes. This is not a kernel thread thing, as > far as I can see. > > If you cannot handle signals, you damn well shouldn't be using > "wait_event_interruptible_timeout()" to begin with. Get rid of the > "interruptible", since it apparently *isn't* interruptible. > > So I'm not pulling this. Ok. It seemed like useful behavior to be able to provide some response to the user in the event that a ->remove handler is blocked by a device in-use and the user attempts to abort the action. Thanks for reviewing, Alex -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [GIT PULL] VFIO fixes for v4.1-rc2
On Fri, May 1, 2015 at 10:40 AM, Alex Williamson wrote: > > - Flush signals on interrupted wait to retain polling interval (Alex > Williamson) This cannot *possibly* be right. If I read this patch right, you're randomly just getting rid of signals. No way in hell is that correct. "flush_signals()" is only for kernel threads, where it's a hacky alternative to actually handling them (since kernel threads never rreturn to user space and cannot really "handle" a signal). But you're doing it in the ->remove handler for the device, which can be called by arbitrary system processes. This is not a kernel thread thing, as far as I can see. If you cannot handle signals, you damn well shouldn't be using "wait_event_interruptible_timeout()" to begin with. Get rid of the "interruptible", since it apparently *isn't* interruptible. So I'm not pulling this. Now I'm worried that other drivers do insane things like this. I wonder if we should add some sanity test to flush_signals() to make sure that it can only ever get called from a kernel thread. Oleg? Linus -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[GIT PULL] VFIO fixes for v4.1-rc2
Hi Linus, The following changes since commit b787f68c36d49bb1d9236f403813641efa74a031: Linux 4.1-rc1 (2015-04-26 17:59:10 -0700) are available in the git repository at: git://github.com/awilliam/linux-vfio.git tags/vfio-v4.1-rc2 for you to fetch changes up to 82a0eaab980a3af92d46e93eaf299d6c1428c52b: vfio-pci: Log device requests more verbosely (2015-04-28 10:23:30 -0600) Fix some undesirable behavior with the vfio device request interface: - Flush signals on interrupted wait to retain polling interval (Alex Williamson) - Increase verbosity of device request channel (Alex Williamson) Alex Williamson (2): vfio: Flush signals on device request interruption vfio-pci: Log device requests more verbosely drivers/vfio/pci/vfio_pci.c | 8 +++- drivers/vfio/vfio.c | 13 ++--- 2 files changed, 17 insertions(+), 4 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[GIT PULL] VFIO fixes for v4.1-rc2
Hi Linus, The following changes since commit b787f68c36d49bb1d9236f403813641efa74a031: Linux 4.1-rc1 (2015-04-26 17:59:10 -0700) are available in the git repository at: git://github.com/awilliam/linux-vfio.git tags/vfio-v4.1-rc2 for you to fetch changes up to 82a0eaab980a3af92d46e93eaf299d6c1428c52b: vfio-pci: Log device requests more verbosely (2015-04-28 10:23:30 -0600) Fix some undesirable behavior with the vfio device request interface: - Flush signals on interrupted wait to retain polling interval (Alex Williamson) - Increase verbosity of device request channel (Alex Williamson) Alex Williamson (2): vfio: Flush signals on device request interruption vfio-pci: Log device requests more verbosely drivers/vfio/pci/vfio_pci.c | 8 +++- drivers/vfio/vfio.c | 13 ++--- 2 files changed, 17 insertions(+), 4 deletions(-) -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [GIT PULL] VFIO fixes for v4.1-rc2
On Fri, May 1, 2015 at 8:37 PM, Linus Torvalds
torva...@linux-foundation.org wrote:
flush_signals() is only for kernel threads, where it's a hacky
alternative to actually handling them (since kernel threads never
rreturn to user space and cannot really handle a signal). But you're
doing it in the -remove handler for the device, which can be called
by arbitrary system processes. This is not a kernel thread thing, as
far as I can see.
If you cannot handle signals, you damn well shouldn't be using
wait_event_interruptible_timeout() to begin with. Get rid of the
interruptible, since it apparently *isn't* interruptible.
So I'm not pulling this.
Now I'm worried that other drivers do insane things like this. I
wonder if we should add some sanity test to flush_signals() to make
sure that it can only ever get called from a kernel thread.
Hmm, a quick grep exposes some questionable users.
At least w1 looks fishy.
drivers/w1/w1_family.c:w1_unregister_family
drivers/w1/w1_int.c:__w1_remove_master_device
What do you think about a WARN_ON like:
diff --git a/kernel/signal.c b/kernel/signal.c
index d51c5dd..b4079c3 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -427,6 +427,8 @@ void flush_signals(struct task_struct *t)
{
unsigned long flags;
+ WARN_ON((current-flags PF_KTHREAD) == 0);
+
spin_lock_irqsave(t-sighand-siglock, flags);
__flush_signals(t);
spin_unlock_irqrestore(t-sighand-siglock, flags);
--
Thanks,
//richard
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [GIT PULL] VFIO fixes for v4.1-rc2
On Fri, May 1, 2015 at 11:48 AM, Alex Williamson alex.william...@redhat.com wrote: Ok. It seemed like useful behavior to be able to provide some response to the user in the event that a -remove handler is blocked by a device in-use and the user attempts to abort the action. Well, that kind of notification *might* be useful, but at the cost of saying somebody tried to send you a signal, so I am now telling you about it, and then deleting that signal, and you'll never know what it actually was? That's not useful, that's just wrong. Now, what might in theory be useful - but I haven't actually seen anybody do anything like that - is to start out with an interruptible sleep, warn if you get interrupted, and then continue with an un-interruptible sleep (leaving the signal active). But even that sounds like a very special case, and I don't think anything has ever done that. In general, our signal handling falls into three distinct categories: (a) interruptible (and you can cancel the operation and return try again) (b) killable (you can cancel the operation, knowing that the requester will be killed and won't try again) (c) uninterruptible where that (b) tends to be a special case of an operation that technically isn't really interruptible (because the ABI doesn't allow for retrying or error returns), but knowing that the caller will never see the error case because it's killed means that you can do it. The classic example of that is an NFS mount that is mounted nointr - you can't return EINTR for a read or a write (because that invalidates POSIX) but you want to let SIGKILL just kill the process in the middle when the network is hung. Linus -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [GIT PULL] VFIO fixes for v4.1-rc2
On Fri, May 1, 2015 at 10:40 AM, Alex Williamson alex.william...@redhat.com wrote: - Flush signals on interrupted wait to retain polling interval (Alex Williamson) This cannot *possibly* be right. If I read this patch right, you're randomly just getting rid of signals. No way in hell is that correct. flush_signals() is only for kernel threads, where it's a hacky alternative to actually handling them (since kernel threads never rreturn to user space and cannot really handle a signal). But you're doing it in the -remove handler for the device, which can be called by arbitrary system processes. This is not a kernel thread thing, as far as I can see. If you cannot handle signals, you damn well shouldn't be using wait_event_interruptible_timeout() to begin with. Get rid of the interruptible, since it apparently *isn't* interruptible. So I'm not pulling this. Now I'm worried that other drivers do insane things like this. I wonder if we should add some sanity test to flush_signals() to make sure that it can only ever get called from a kernel thread. Oleg? Linus -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [GIT PULL] VFIO fixes for v4.1-rc2
On Fri, 2015-05-01 at 11:37 -0700, Linus Torvalds wrote: On Fri, May 1, 2015 at 10:40 AM, Alex Williamson alex.william...@redhat.com wrote: - Flush signals on interrupted wait to retain polling interval (Alex Williamson) This cannot *possibly* be right. If I read this patch right, you're randomly just getting rid of signals. No way in hell is that correct. flush_signals() is only for kernel threads, where it's a hacky alternative to actually handling them (since kernel threads never rreturn to user space and cannot really handle a signal). But you're doing it in the -remove handler for the device, which can be called by arbitrary system processes. This is not a kernel thread thing, as far as I can see. If you cannot handle signals, you damn well shouldn't be using wait_event_interruptible_timeout() to begin with. Get rid of the interruptible, since it apparently *isn't* interruptible. So I'm not pulling this. Ok. It seemed like useful behavior to be able to provide some response to the user in the event that a -remove handler is blocked by a device in-use and the user attempts to abort the action. Thanks for reviewing, Alex -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [GIT PULL] VFIO fixes for v4.1-rc2
...of course I meant t- and not current-
On 5/1/15, Richard Weinberger richard.weinber...@gmail.com wrote:
On Fri, May 1, 2015 at 8:37 PM, Linus Torvalds
torva...@linux-foundation.org wrote:
flush_signals() is only for kernel threads, where it's a hacky
alternative to actually handling them (since kernel threads never
rreturn to user space and cannot really handle a signal). But you're
doing it in the -remove handler for the device, which can be called
by arbitrary system processes. This is not a kernel thread thing, as
far as I can see.
If you cannot handle signals, you damn well shouldn't be using
wait_event_interruptible_timeout() to begin with. Get rid of the
interruptible, since it apparently *isn't* interruptible.
So I'm not pulling this.
Now I'm worried that other drivers do insane things like this. I
wonder if we should add some sanity test to flush_signals() to make
sure that it can only ever get called from a kernel thread.
Hmm, a quick grep exposes some questionable users.
At least w1 looks fishy.
drivers/w1/w1_family.c:w1_unregister_family
drivers/w1/w1_int.c:__w1_remove_master_device
What do you think about a WARN_ON like:
diff --git a/kernel/signal.c b/kernel/signal.c
index d51c5dd..b4079c3 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -427,6 +427,8 @@ void flush_signals(struct task_struct *t)
{
unsigned long flags;
+ WARN_ON((current-flags PF_KTHREAD) == 0);
+
spin_lock_irqsave(t-sighand-siglock, flags);
__flush_signals(t);
spin_unlock_irqrestore(t-sighand-siglock, flags);
--
Thanks,
//richard
--
Thanks,
//richard
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [GIT PULL] VFIO fixes for v4.1-rc2
On Fri, 2015-05-01 at 13:23 -0700, Linus Torvalds wrote: On Fri, May 1, 2015 at 11:48 AM, Alex Williamson alex.william...@redhat.com wrote: Ok. It seemed like useful behavior to be able to provide some response to the user in the event that a -remove handler is blocked by a device in-use and the user attempts to abort the action. Well, that kind of notification *might* be useful, but at the cost of saying somebody tried to send you a signal, so I am now telling you about it, and then deleting that signal, and you'll never know what it actually was? That's not useful, that's just wrong. Yep, it was a bad idea. Now, what might in theory be useful - but I haven't actually seen anybody do anything like that - is to start out with an interruptible sleep, warn if you get interrupted, and then continue with an un-interruptible sleep (leaving the signal active). I was considering doing exactly this. But even that sounds like a very special case, and I don't think anything has ever done that. In general, our signal handling falls into three distinct categories: (a) interruptible (and you can cancel the operation and return try again) (b) killable (you can cancel the operation, knowing that the requester will be killed and won't try again) (c) uninterruptible where that (b) tends to be a special case of an operation that technically isn't really interruptible (because the ABI doesn't allow for retrying or error returns), but knowing that the caller will never see the error case because it's killed means that you can do it. The classic example of that is an NFS mount that is mounted nointr - you can't return EINTR for a read or a write (because that invalidates POSIX) but you want to let SIGKILL just kill the process in the middle when the network is hung. I think we're in that (c) case unless we want to change our driver API to allow driver remove callbacks to return error. Killing the caller doesn't really help the situation without being able to back out of the remove path. Killing the task with the device open would help, but seems rather harsh. I expect we eventually want to be able to escalate to revoking the device from the user, but currently we only have a notifier to request the device from cooperative users. In the event of an uncooperative user, we block, which can be difficult to figure out, especially when we're dealing with SR-IOV devices and a PF unbind implicitly induces a VF unbind. The interruptible component here is simply a logging mechanism which should have turned into an interruptible_once rather than a signal flush. I try to avoid vfio being a special case, but maybe in this instance it's worthwhile. If you have other suggestions, please let me know. Thanks, Alex -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
