| Message ID | 20150827162529.GA7893@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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(¤t->sighand->siglock, flags); > - current->notifier_mask = mask; > - current->notifier_data = priv; > - current->notifier = notifier; > - spin_unlock_irqrestore(¤t->sighand->siglock, flags); > -} > - > -/* Notify the system that blocking has ended. */ > - > -void > -unblock_all_signals(void) > -{ > - unsigned long flags; > - > - spin_lock_irqsave(¤t->sighand->siglock, flags); > - current->notifier = NULL; > - current->notifier_data = NULL; > - recalc_sigpending(); > - spin_unlock_irqrestore(¤t->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
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(¤t->sighand->siglock, flags); > - current->notifier_mask = mask; > - current->notifier_data = priv; > - current->notifier = notifier; > - spin_unlock_irqrestore(¤t->sighand->siglock, flags); > -} > - > -/* Notify the system that blocking has ended. */ > - > -void > -unblock_all_signals(void) > -{ > - unsigned long flags; > - > - spin_lock_irqsave(¤t->sighand->siglock, flags); > - current->notifier = NULL; > - current->notifier_data = NULL; > - recalc_sigpending(); > - spin_unlock_irqrestore(¤t->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 >
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.
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(¤t->sighand->siglock, flags); - current->notifier_mask = mask; - current->notifier_data = priv; - current->notifier = notifier; - spin_unlock_irqrestore(¤t->sighand->siglock, flags); -} - -/* Notify the system that blocking has ended. */ - -void -unblock_all_signals(void) -{ - unsigned long flags; - - spin_lock_irqsave(¤t->sighand->siglock, flags); - current->notifier = NULL; - current->notifier_data = NULL; - recalc_sigpending(); - spin_unlock_irqrestore(¤t->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.
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(-)