Re: [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 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