[PATCH 1/1] signals: kill block_all_signals() and unblock_all_signals()
It is hardly possible to enumerate all problems with block_all_signals()
and unblock_all_signals(). Just for example,
1. block_all_signals(SIGSTOP/etc) simply can't help if the caller is
multithreaded. Another thread can dequeue the signal and force the
group stop.
2. Even is the caller is single-threaded, it will "stop" anyway. It
will not sleep, but it will spin in kernel space until SIGCONT or
SIGKILL.
And a lot more. In short, this interface doesn't work at all, at least
the last 10+ years.
Signed-off-by: Oleg Nesterov
---
drivers/gpu/drm/drm_lock.c | 41 ---
include/drm/drmP.h |1 -
include/linux/sched.h |7 +-
kernel/signal.c| 51 +---
4 files changed, 2 insertions(+), 98 deletions(-)
diff --git a/drivers/gpu/drm/drm_lock.c b/drivers/gpu/drm/drm_lock.c
index f861361..4477b87 100644
--- a/drivers/gpu/drm/drm_lock.c
+++ b/drivers/gpu/drm/drm_lock.c
@@ -38,8 +38,6 @@
#include "drm_legacy.h"
#include "drm_internal.h"
-static int drm_notifier(void *priv);
-
static int drm_lock_take(struct drm_lock_data *lock_data, unsigned int
context);
/**
@@ -115,14 +113,8 @@ int drm_legacy_lock(struct drm_device *dev, void *data,
* really probably not the correct answer but lets us debug xkb
* xserver for now */
if (!file_priv->is_master) {
- sigemptyset(&dev->sigmask);
- sigaddset(&dev->sigmask, SIGSTOP);
- sigaddset(&dev->sigmask, SIGTSTP);
- sigaddset(&dev->sigmask, SIGTTIN);
- sigaddset(&dev->sigmask, SIGTTOU);
dev->sigdata.context = lock->context;
dev->sigdata.lock = master->lock.hw_lock;
- block_all_signals(drm_notifier, dev, &dev->sigmask);
}
if (dev->driver->dma_quiescent && (lock->flags & _DRM_LOCK_QUIESCENT))
@@ -163,7 +155,6 @@ int drm_legacy_unlock(struct drm_device *dev, void *data,
struct drm_file *file_
/* FIXME: Should really bail out here. */
}
- unblock_all_signals();
return 0;
}
@@ -282,38 +273,6 @@ int drm_legacy_lock_free(struct drm_lock_data *lock_data,
unsigned int context)
}
/**
- * If we get here, it means that the process has called DRM_IOCTL_LOCK
- * without calling DRM_IOCTL_UNLOCK.
- *
- * If the lock is not held, then let the signal proceed as usual. If the lock
- * is held, then set the contended flag and keep the signal blocked.
- *
- * \param priv pointer to a drm_device structure.
- * \return one if the signal should be delivered normally, or zero if the
- * signal should be blocked.
- */
-static int drm_notifier(void *priv)
-{
- struct drm_device *dev = priv;
- struct drm_hw_lock *lock = dev->sigdata.lock;
- unsigned int old, new, prev;
-
- /* Allow signal delivery if lock isn't held */
- if (!lock || !_DRM_LOCK_IS_HELD(lock->lock)
- || _DRM_LOCKING_CONTEXT(lock->lock) != dev->sigdata.context)
- return 1;
-
- /* Otherwise, set flag to force call to
- drmUnlock */
- do {
- old = lock->lock;
- new = old | _DRM_LOCK_CONT;
- prev = cmpxchg(&lock->lock, old, new);
- } while (prev != old);
- return 0;
-}
-
-/**
* This function returns immediately and takes the hw lock
* with the kernel context if it is free, otherwise it gets the highest
priority when and if
* it is eventually released.
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 62c4077..0859c35 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -815,7 +815,6 @@ struct drm_device {
struct drm_sg_mem *sg; /**< Scatter gather memory */
unsigned int num_crtcs; /**< Number of CRTCs on this
device */
- sigset_t sigmask;
struct {
int context;
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 26a2e61..f192cfe 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1489,9 +1489,7 @@ struct task_struct {
unsigned long sas_ss_sp;
size_t sas_ss_size;
- int (*notifier)(void *priv);
- void *notifier_data;
- sigset_t *notifier_mask;
+
struct callback_head *task_works;
struct audit_context *audit_context;
@@ -2382,9 +2380,6 @@ static inline int dequeue_signal_lock(struct task_struct
*tsk, sigset_t *mask, s
return ret;
}
-extern void block_all_signals(int (*notifier)(void *priv), void *priv,
- sigset_t *mask);
-extern void unblock_all_signals(void);
extern void release_task(struct task_struct * p);
extern int send_sig_info(int, struct siginfo *, struct task_struct *);
extern int force_sigsegv(int, struct task_struct *);
diff --git a/kernel/signal.c b/kernel/signal.c
index d51c5dd..a5f4f85 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -508,41 +508,6 @@ int unhandled_sig
[PATCH 1/1] signals: kill block_all_signals() and unblock_all_signals()
On Thu, Aug 27, 2015 at 06:25:29PM +0200, Oleg Nesterov wrote:
> It is hardly possible to enumerate all problems with block_all_signals()
> and unblock_all_signals(). Just for example,
>
> 1. block_all_signals(SIGSTOP/etc) simply can't help if the caller is
>multithreaded. Another thread can dequeue the signal and force the
>group stop.
>
> 2. Even is the caller is single-threaded, it will "stop" anyway. It
>will not sleep, but it will spin in kernel space until SIGCONT or
>SIGKILL.
>
> And a lot more. In short, this interface doesn't work at all, at least
> the last 10+ years.
>
> Signed-off-by: Oleg Nesterov
Yeah the only times I played around with the DRM_LOCK stuff was when old
drivers accidentally deadlocked - my impression is that the entire
DRM_LOCK thing was never really tested properly ;-) Hence I'm all for
purging where this leaks out of the drm subsystem.
Acked-by: Daniel Vetter
> ---
> drivers/gpu/drm/drm_lock.c | 41 ---
> include/drm/drmP.h |1 -
> include/linux/sched.h |7 +-
> kernel/signal.c| 51
> +---
> 4 files changed, 2 insertions(+), 98 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_lock.c b/drivers/gpu/drm/drm_lock.c
> index f861361..4477b87 100644
> --- a/drivers/gpu/drm/drm_lock.c
> +++ b/drivers/gpu/drm/drm_lock.c
> @@ -38,8 +38,6 @@
> #include "drm_legacy.h"
> #include "drm_internal.h"
>
> -static int drm_notifier(void *priv);
> -
> static int drm_lock_take(struct drm_lock_data *lock_data, unsigned int
> context);
>
> /**
> @@ -115,14 +113,8 @@ int drm_legacy_lock(struct drm_device *dev, void *data,
>* really probably not the correct answer but lets us debug xkb
>* xserver for now */
> if (!file_priv->is_master) {
> - sigemptyset(&dev->sigmask);
> - sigaddset(&dev->sigmask, SIGSTOP);
> - sigaddset(&dev->sigmask, SIGTSTP);
> - sigaddset(&dev->sigmask, SIGTTIN);
> - sigaddset(&dev->sigmask, SIGTTOU);
> dev->sigdata.context = lock->context;
> dev->sigdata.lock = master->lock.hw_lock;
> - block_all_signals(drm_notifier, dev, &dev->sigmask);
> }
>
> if (dev->driver->dma_quiescent && (lock->flags & _DRM_LOCK_QUIESCENT))
> @@ -163,7 +155,6 @@ int drm_legacy_unlock(struct drm_device *dev, void *data,
> struct drm_file *file_
> /* FIXME: Should really bail out here. */
> }
>
> - unblock_all_signals();
> return 0;
> }
>
> @@ -282,38 +273,6 @@ int drm_legacy_lock_free(struct drm_lock_data
> *lock_data, unsigned int context)
> }
>
> /**
> - * If we get here, it means that the process has called DRM_IOCTL_LOCK
> - * without calling DRM_IOCTL_UNLOCK.
> - *
> - * If the lock is not held, then let the signal proceed as usual. If the
> lock
> - * is held, then set the contended flag and keep the signal blocked.
> - *
> - * \param priv pointer to a drm_device structure.
> - * \return one if the signal should be delivered normally, or zero if the
> - * signal should be blocked.
> - */
> -static int drm_notifier(void *priv)
> -{
> - struct drm_device *dev = priv;
> - struct drm_hw_lock *lock = dev->sigdata.lock;
> - unsigned int old, new, prev;
> -
> - /* Allow signal delivery if lock isn't held */
> - if (!lock || !_DRM_LOCK_IS_HELD(lock->lock)
> - || _DRM_LOCKING_CONTEXT(lock->lock) != dev->sigdata.context)
> - return 1;
> -
> - /* Otherwise, set flag to force call to
> -drmUnlock */
> - do {
> - old = lock->lock;
> - new = old | _DRM_LOCK_CONT;
> - prev = cmpxchg(&lock->lock, old, new);
> - } while (prev != old);
> - return 0;
> -}
> -
> -/**
> * This function returns immediately and takes the hw lock
> * with the kernel context if it is free, otherwise it gets the highest
> priority when and if
> * it is eventually released.
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index 62c4077..0859c35 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -815,7 +815,6 @@ struct drm_device {
>
> struct drm_sg_mem *sg; /**< Scatter gather memory */
> unsigned int num_crtcs; /**< Number of CRTCs on this
> device */
> - sigset_t sigmask;
>
> struct {
> int context;
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 26a2e61..f192cfe 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -1489,9 +1489,7 @@ struct task_struct {
>
> unsigned long sas_ss_sp;
> size_t sas_ss_size;
> - int (*notifier)(void *priv);
> - void *notifier_data;
> - sigset_t *notifier_mask;
> +
> struct callback_head *task_works;
>
> struct audit_context *audit_context;
> @@ -2382,9 +2380,6 @@ static inline int dequeue_signal_lock(struct
> task_struct *t
[PATCH 1/1] signals: kill block_all_signals() and unblock_all_signals()
ping ;)
Andrew, should I re-send this patch? It was acked by Daniel and Dave
doesn't object.
Dave, I'll appreciate it if you ack it explicitly.
On 08/27, Oleg Nesterov wrote:
>
> It is hardly possible to enumerate all problems with block_all_signals()
> and unblock_all_signals(). Just for example,
>
> 1. block_all_signals(SIGSTOP/etc) simply can't help if the caller is
>multithreaded. Another thread can dequeue the signal and force the
>group stop.
>
> 2. Even is the caller is single-threaded, it will "stop" anyway. It
>will not sleep, but it will spin in kernel space until SIGCONT or
>SIGKILL.
>
> And a lot more. In short, this interface doesn't work at all, at least
> the last 10+ years.
>
> Signed-off-by: Oleg Nesterov
> ---
> drivers/gpu/drm/drm_lock.c | 41 ---
> include/drm/drmP.h |1 -
> include/linux/sched.h |7 +-
> kernel/signal.c| 51
> +---
> 4 files changed, 2 insertions(+), 98 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_lock.c b/drivers/gpu/drm/drm_lock.c
> index f861361..4477b87 100644
> --- a/drivers/gpu/drm/drm_lock.c
> +++ b/drivers/gpu/drm/drm_lock.c
> @@ -38,8 +38,6 @@
> #include "drm_legacy.h"
> #include "drm_internal.h"
>
> -static int drm_notifier(void *priv);
> -
> static int drm_lock_take(struct drm_lock_data *lock_data, unsigned int
> context);
>
> /**
> @@ -115,14 +113,8 @@ int drm_legacy_lock(struct drm_device *dev, void *data,
>* really probably not the correct answer but lets us debug xkb
>* xserver for now */
> if (!file_priv->is_master) {
> - sigemptyset(&dev->sigmask);
> - sigaddset(&dev->sigmask, SIGSTOP);
> - sigaddset(&dev->sigmask, SIGTSTP);
> - sigaddset(&dev->sigmask, SIGTTIN);
> - sigaddset(&dev->sigmask, SIGTTOU);
> dev->sigdata.context = lock->context;
> dev->sigdata.lock = master->lock.hw_lock;
> - block_all_signals(drm_notifier, dev, &dev->sigmask);
> }
>
> if (dev->driver->dma_quiescent && (lock->flags & _DRM_LOCK_QUIESCENT))
> @@ -163,7 +155,6 @@ int drm_legacy_unlock(struct drm_device *dev, void *data,
> struct drm_file *file_
> /* FIXME: Should really bail out here. */
> }
>
> - unblock_all_signals();
> return 0;
> }
>
> @@ -282,38 +273,6 @@ int drm_legacy_lock_free(struct drm_lock_data
> *lock_data, unsigned int context)
> }
>
> /**
> - * If we get here, it means that the process has called DRM_IOCTL_LOCK
> - * without calling DRM_IOCTL_UNLOCK.
> - *
> - * If the lock is not held, then let the signal proceed as usual. If the
> lock
> - * is held, then set the contended flag and keep the signal blocked.
> - *
> - * \param priv pointer to a drm_device structure.
> - * \return one if the signal should be delivered normally, or zero if the
> - * signal should be blocked.
> - */
> -static int drm_notifier(void *priv)
> -{
> - struct drm_device *dev = priv;
> - struct drm_hw_lock *lock = dev->sigdata.lock;
> - unsigned int old, new, prev;
> -
> - /* Allow signal delivery if lock isn't held */
> - if (!lock || !_DRM_LOCK_IS_HELD(lock->lock)
> - || _DRM_LOCKING_CONTEXT(lock->lock) != dev->sigdata.context)
> - return 1;
> -
> - /* Otherwise, set flag to force call to
> -drmUnlock */
> - do {
> - old = lock->lock;
> - new = old | _DRM_LOCK_CONT;
> - prev = cmpxchg(&lock->lock, old, new);
> - } while (prev != old);
> - return 0;
> -}
> -
> -/**
> * This function returns immediately and takes the hw lock
> * with the kernel context if it is free, otherwise it gets the highest
> priority when and if
> * it is eventually released.
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index 62c4077..0859c35 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -815,7 +815,6 @@ struct drm_device {
>
> struct drm_sg_mem *sg; /**< Scatter gather memory */
> unsigned int num_crtcs; /**< Number of CRTCs on this
> device */
> - sigset_t sigmask;
>
> struct {
> int context;
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 26a2e61..f192cfe 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -1489,9 +1489,7 @@ struct task_struct {
>
> unsigned long sas_ss_sp;
> size_t sas_ss_size;
> - int (*notifier)(void *priv);
> - void *notifier_data;
> - sigset_t *notifier_mask;
> +
> struct callback_head *task_works;
>
> struct audit_context *audit_context;
> @@ -2382,9 +2380,6 @@ static inline int dequeue_signal_lock(struct
> task_struct *tsk, sigset_t *mask, s
> return ret;
> }
>
> -extern void block_all_signals(int (*notifier)(void *priv), void *priv,
> - sigset_t *mask)
[PATCH 1/1] signals: kill block_all_signals() and unblock_all_signals()
On 16 September 2015 at 02:41, Oleg Nesterov wrote: > ping ;) > > Andrew, should I re-send this patch? It was acked by Daniel and Dave > doesn't object. > > Dave, I'll appreciate it if you ack it explicitly. > > > On 08/27, Oleg Nesterov wrote: >> >> It is hardly possible to enumerate all problems with block_all_signals() >> and unblock_all_signals(). Just for example, >> >> 1. block_all_signals(SIGSTOP/etc) simply can't help if the caller is >>multithreaded. Another thread can dequeue the signal and force the >>group stop. >> >> 2. Even is the caller is single-threaded, it will "stop" anyway. It >>will not sleep, but it will spin in kernel space until SIGCONT or >>SIGKILL. >> >> And a lot more. In short, this interface doesn't work at all, at least >> the last 10+ years. >> >> Signed-off-by: Oleg Nesterov Acked-by: Dave Airlie Probably best to go via Andrew alright. Dave.
