[PATCH 0/1] signals: kill block_all_signals() and unblock_all_signals()

[PATCH 0/1] signals: kill block_all_signals() and unblock_all_signals()

[Oleg Nesterov]

On 05/26, Dave Airlie wrote:
>
> On 26 May 2015 at 02:50, Oleg Nesterov <oleg@redhat.com> wrote:
> > AAAAOn 05/25, Richard Weinberger wrote:
> >>
> >> Is this functionality still in use/needed?
> >
> > All I can say it doesn't work.
> >
> >> Otherwise we could get rid of block_all_signals() and unpuzzle the signaling
> >> code a bit. :-)
> >
> > Yes. I do not even remember when I reported this the first time. Perhaps
> > more than 10 years ago.
> >
> > See the last attempt in 2011: https://lkml.org/lkml/2011/7/12/263
> > I copied this email below.
> >
> > Dave. Lets finally kill this horror? I am going to send a patch unless
> > you stop me ;)
>
> There were follow up on that thread 4 years ago, but we are probably
> at the stage where this thing can die,

Heh.

I tried to kill this horror so many years, and forgot to send the patch
after you finally blessed it ;)

Could you please review?

> I suspect any hw using it has died out, and any new hardware won't be
> doing evil things with drm locks

even if it does, let me repeat that block_all_signals() simply do not
work. At all. if ->notifier() returns 0, this thread will burn CPU until
SIGCONT or SIGKILL. Or it will stop anyway if multi-threaded.

Oleg.

[PATCH 1/1] signals: kill block_all_signals() and unblock_all_signals()

[Oleg Nesterov]

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 <oleg@redhat.com>
---
 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_signal(struct task_struct *tsk, int sig)
 	return !tsk->ptrace;
 }
 
-/*
- * Notify the system that a driver wants to block all signals for this
- * process, and wants to be notified if any signals at all were to be
- * sent/acted upon.  If the notifier routine returns non-zero, then the
- * signal will be acted upon after all.  If the notifier routine returns 0,
- * then then signal will be blocked.  Only one block per process is
- * allowed.  priv is a pointer to private data that the notifier routine
- * can use to determine if the signal should be blocked or not.
- */
-void
-block_all_signals(int (*notifier)(void *priv), void *priv, sigset_t *mask)
-{
-	unsigned long flags;
-
-	spin_lock_irqsave(&current->sighand->siglock, flags);
-	current->notifier_mask = mask;
-	current->notifier_data = priv;
-	current->notifier = notifier;
-	spin_unlock_irqrestore(&current->sighand->siglock, flags);
-}
-
-/* Notify the system that blocking has ended. */
-
-void
-unblock_all_signals(void)
-{
-	unsigned long flags;
-
-	spin_lock_irqsave(&current->sighand->siglock, flags);
-	current->notifier = NULL;
-	current->notifier_data = NULL;
-	recalc_sigpending();
-	spin_unlock_irqrestore(&current->sighand->siglock, flags);
-}
-
 static void collect_signal(int sig, struct sigpending *list, siginfo_t *info)
 {
 	struct sigqueue *q, *first = NULL;
@@ -585,19 +550,8 @@ static int __dequeue_signal(struct sigpending *pending, sigset_t *mask,
 {
 	int sig = next_signal(pending, mask);
 
-	if (sig) {
-		if (current->notifier) {
-			if (sigismember(current->notifier_mask, sig)) {
-				if (!(current->notifier)(current->notifier_data)) {
-					clear_thread_flag(TIF_SIGPENDING);
-					return 0;
-				}
-			}
-		}
-
+	if (sig)
 		collect_signal(sig, pending, info);
-	}
-
 	return sig;
 }
 
@@ -2488,9 +2442,6 @@ EXPORT_SYMBOL(force_sig);
 EXPORT_SYMBOL(send_sig);
 EXPORT_SYMBOL(send_sig_info);
 EXPORT_SYMBOL(sigprocmask);
-EXPORT_SYMBOL(block_all_signals);
-EXPORT_SYMBOL(unblock_all_signals);
-
 
 /*
  * System call entry points.
-- 
1.5.5.1

Re: [PATCH 1/1] signals: kill block_all_signals() and unblock_all_signals()

[Daniel Vetter]

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 <oleg@redhat.com>

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 <daniel.vetter@ffwll.ch>
> ---
>  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_signal(struct task_struct *tsk, int sig)
>  	return !tsk->ptrace;
>  }
>  
> -/*
> - * Notify the system that a driver wants to block all signals for this
> - * process, and wants to be notified if any signals at all were to be
> - * sent/acted upon.  If the notifier routine returns non-zero, then the
> - * signal will be acted upon after all.  If the notifier routine returns 0,
> - * then then signal will be blocked.  Only one block per process is
> - * allowed.  priv is a pointer to private data that the notifier routine
> - * can use to determine if the signal should be blocked or not.
> - */
> -void
> -block_all_signals(int (*notifier)(void *priv), void *priv, sigset_t *mask)
> -{
> -	unsigned long flags;
> -
> -	spin_lock_irqsave(&current->sighand->siglock, flags);
> -	current->notifier_mask = mask;
> -	current->notifier_data = priv;
> -	current->notifier = notifier;
> -	spin_unlock_irqrestore(&current->sighand->siglock, flags);
> -}
> -
> -/* Notify the system that blocking has ended. */
> -
> -void
> -unblock_all_signals(void)
> -{
> -	unsigned long flags;
> -
> -	spin_lock_irqsave(&current->sighand->siglock, flags);
> -	current->notifier = NULL;
> -	current->notifier_data = NULL;
> -	recalc_sigpending();
> -	spin_unlock_irqrestore(&current->sighand->siglock, flags);
> -}
> -
>  static void collect_signal(int sig, struct sigpending *list, siginfo_t *info)
>  {
>  	struct sigqueue *q, *first = NULL;
> @@ -585,19 +550,8 @@ static int __dequeue_signal(struct sigpending *pending, sigset_t *mask,
>  {
>  	int sig = next_signal(pending, mask);
>  
> -	if (sig) {
> -		if (current->notifier) {
> -			if (sigismember(current->notifier_mask, sig)) {
> -				if (!(current->notifier)(current->notifier_data)) {
> -					clear_thread_flag(TIF_SIGPENDING);
> -					return 0;
> -				}
> -			}
> -		}
> -
> +	if (sig)
>  		collect_signal(sig, pending, info);
> -	}
> -
>  	return sig;
>  }
>  
> @@ -2488,9 +2442,6 @@ EXPORT_SYMBOL(force_sig);
>  EXPORT_SYMBOL(send_sig);
>  EXPORT_SYMBOL(send_sig_info);
>  EXPORT_SYMBOL(sigprocmask);
> -EXPORT_SYMBOL(block_all_signals);
> -EXPORT_SYMBOL(unblock_all_signals);
> -
>  
>  /*
>   * System call entry points.
> -- 
> 1.5.5.1
> 
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/1] signals: kill block_all_signals() and unblock_all_signals()

[Daniel Vetter]

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 <oleg@redhat.com>

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 <daniel.vetter@ffwll.ch>
> ---
>  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_signal(struct task_struct *tsk, int sig)
>  	return !tsk->ptrace;
>  }
>  
> -/*
> - * Notify the system that a driver wants to block all signals for this
> - * process, and wants to be notified if any signals at all were to be
> - * sent/acted upon.  If the notifier routine returns non-zero, then the
> - * signal will be acted upon after all.  If the notifier routine returns 0,
> - * then then signal will be blocked.  Only one block per process is
> - * allowed.  priv is a pointer to private data that the notifier routine
> - * can use to determine if the signal should be blocked or not.
> - */
> -void
> -block_all_signals(int (*notifier)(void *priv), void *priv, sigset_t *mask)
> -{
> -	unsigned long flags;
> -
> -	spin_lock_irqsave(&current->sighand->siglock, flags);
> -	current->notifier_mask = mask;
> -	current->notifier_data = priv;
> -	current->notifier = notifier;
> -	spin_unlock_irqrestore(&current->sighand->siglock, flags);
> -}
> -
> -/* Notify the system that blocking has ended. */
> -
> -void
> -unblock_all_signals(void)
> -{
> -	unsigned long flags;
> -
> -	spin_lock_irqsave(&current->sighand->siglock, flags);
> -	current->notifier = NULL;
> -	current->notifier_data = NULL;
> -	recalc_sigpending();
> -	spin_unlock_irqrestore(&current->sighand->siglock, flags);
> -}
> -
>  static void collect_signal(int sig, struct sigpending *list, siginfo_t *info)
>  {
>  	struct sigqueue *q, *first = NULL;
> @@ -585,19 +550,8 @@ static int __dequeue_signal(struct sigpending *pending, sigset_t *mask,
>  {
>  	int sig = next_signal(pending, mask);
>  
> -	if (sig) {
> -		if (current->notifier) {
> -			if (sigismember(current->notifier_mask, sig)) {
> -				if (!(current->notifier)(current->notifier_data)) {
> -					clear_thread_flag(TIF_SIGPENDING);
> -					return 0;
> -				}
> -			}
> -		}
> -
> +	if (sig)
>  		collect_signal(sig, pending, info);
> -	}
> -
>  	return sig;
>  }
>  
> @@ -2488,9 +2442,6 @@ EXPORT_SYMBOL(force_sig);
>  EXPORT_SYMBOL(send_sig);
>  EXPORT_SYMBOL(send_sig_info);
>  EXPORT_SYMBOL(sigprocmask);
> -EXPORT_SYMBOL(block_all_signals);
> -EXPORT_SYMBOL(unblock_all_signals);
> -
>  
>  /*
>   * System call entry points.
> -- 
> 1.5.5.1
> 
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 1/1] signals: kill block_all_signals() and unblock_all_signals()

[Oleg Nesterov]

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 <oleg@redhat.com>
> ---
>  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_signal(struct task_struct *tsk, int sig)
>  	return !tsk->ptrace;
>  }
>  
> -/*
> - * Notify the system that a driver wants to block all signals for this
> - * process, and wants to be notified if any signals at all were to be
> - * sent/acted upon.  If the notifier routine returns non-zero, then the
> - * signal will be acted upon after all.  If the notifier routine returns 0,
> - * then then signal will be blocked.  Only one block per process is
> - * allowed.  priv is a pointer to private data that the notifier routine
> - * can use to determine if the signal should be blocked or not.
> - */
> -void
> -block_all_signals(int (*notifier)(void *priv), void *priv, sigset_t *mask)
> -{
> -	unsigned long flags;
> -
> -	spin_lock_irqsave(&current->sighand->siglock, flags);
> -	current->notifier_mask = mask;
> -	current->notifier_data = priv;
> -	current->notifier = notifier;
> -	spin_unlock_irqrestore(&current->sighand->siglock, flags);
> -}
> -
> -/* Notify the system that blocking has ended. */
> -
> -void
> -unblock_all_signals(void)
> -{
> -	unsigned long flags;
> -
> -	spin_lock_irqsave(&current->sighand->siglock, flags);
> -	current->notifier = NULL;
> -	current->notifier_data = NULL;
> -	recalc_sigpending();
> -	spin_unlock_irqrestore(&current->sighand->siglock, flags);
> -}
> -
>  static void collect_signal(int sig, struct sigpending *list, siginfo_t *info)
>  {
>  	struct sigqueue *q, *first = NULL;
> @@ -585,19 +550,8 @@ static int __dequeue_signal(struct sigpending *pending, sigset_t *mask,
>  {
>  	int sig = next_signal(pending, mask);
>  
> -	if (sig) {
> -		if (current->notifier) {
> -			if (sigismember(current->notifier_mask, sig)) {
> -				if (!(current->notifier)(current->notifier_data)) {
> -					clear_thread_flag(TIF_SIGPENDING);
> -					return 0;
> -				}
> -			}
> -		}
> -
> +	if (sig)
>  		collect_signal(sig, pending, info);
> -	}
> -
>  	return sig;
>  }
>  
> @@ -2488,9 +2442,6 @@ EXPORT_SYMBOL(force_sig);
>  EXPORT_SYMBOL(send_sig);
>  EXPORT_SYMBOL(send_sig_info);
>  EXPORT_SYMBOL(sigprocmask);
> -EXPORT_SYMBOL(block_all_signals);
> -EXPORT_SYMBOL(unblock_all_signals);
> -
>  
>  /*
>   * System call entry points.
> -- 
> 1.5.5.1
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/1] signals: kill block_all_signals() and unblock_all_signals()

[Oleg Nesterov]

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 <oleg@redhat.com>
> ---
>  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_signal(struct task_struct *tsk, int sig)
>  	return !tsk->ptrace;
>  }
>  
> -/*
> - * Notify the system that a driver wants to block all signals for this
> - * process, and wants to be notified if any signals at all were to be
> - * sent/acted upon.  If the notifier routine returns non-zero, then the
> - * signal will be acted upon after all.  If the notifier routine returns 0,
> - * then then signal will be blocked.  Only one block per process is
> - * allowed.  priv is a pointer to private data that the notifier routine
> - * can use to determine if the signal should be blocked or not.
> - */
> -void
> -block_all_signals(int (*notifier)(void *priv), void *priv, sigset_t *mask)
> -{
> -	unsigned long flags;
> -
> -	spin_lock_irqsave(&current->sighand->siglock, flags);
> -	current->notifier_mask = mask;
> -	current->notifier_data = priv;
> -	current->notifier = notifier;
> -	spin_unlock_irqrestore(&current->sighand->siglock, flags);
> -}
> -
> -/* Notify the system that blocking has ended. */
> -
> -void
> -unblock_all_signals(void)
> -{
> -	unsigned long flags;
> -
> -	spin_lock_irqsave(&current->sighand->siglock, flags);
> -	current->notifier = NULL;
> -	current->notifier_data = NULL;
> -	recalc_sigpending();
> -	spin_unlock_irqrestore(&current->sighand->siglock, flags);
> -}
> -
>  static void collect_signal(int sig, struct sigpending *list, siginfo_t *info)
>  {
>  	struct sigqueue *q, *first = NULL;
> @@ -585,19 +550,8 @@ static int __dequeue_signal(struct sigpending *pending, sigset_t *mask,
>  {
>  	int sig = next_signal(pending, mask);
>  
> -	if (sig) {
> -		if (current->notifier) {
> -			if (sigismember(current->notifier_mask, sig)) {
> -				if (!(current->notifier)(current->notifier_data)) {
> -					clear_thread_flag(TIF_SIGPENDING);
> -					return 0;
> -				}
> -			}
> -		}
> -
> +	if (sig)
>  		collect_signal(sig, pending, info);
> -	}
> -
>  	return sig;
>  }
>  
> @@ -2488,9 +2442,6 @@ EXPORT_SYMBOL(force_sig);
>  EXPORT_SYMBOL(send_sig);
>  EXPORT_SYMBOL(send_sig_info);
>  EXPORT_SYMBOL(sigprocmask);
> -EXPORT_SYMBOL(block_all_signals);
> -EXPORT_SYMBOL(unblock_all_signals);
> -
>  
>  /*
>   * System call entry points.
> -- 
> 1.5.5.1
> 

Re: [PATCH 1/1] signals: kill block_all_signals() and unblock_all_signals()

[Dave Airlie]

On 16 September 2015 at 02:41, Oleg Nesterov <oleg@redhat.com> 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 <oleg@redhat.com>

Acked-by: Dave Airlie <airlied@redhat.com>

Probably best to go via Andrew alright.

Dave.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/1] signals: kill block_all_signals() and unblock_all_signals()

[Dave Airlie]

On 16 September 2015 at 02:41, Oleg Nesterov <oleg@redhat.com> 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 <oleg@redhat.com>

Acked-by: Dave Airlie <airlied@redhat.com>

Probably best to go via Andrew alright.

Dave.