On Fri, Aug 09, 2013 at 05:19:13PM +0200, Oleg Nesterov wrote:
> device_close()->recalc_sigpending() is not needed, sigprocmask()
> takes care of TIF_SIGPENDING correctly.
>
> And without ->siglock it is racy and wrong, it can wrongly clear
> TIF_SIGPENDING and miss a signal.
>
> But even with this patch device_close() is still buggy:
>
> 1. sigprocmask() should not be used, we have set_task_blocked(),
> but this is minor.
>
> 2. We should never block SIGKILL or SIGSTOP, and this is what
> the code tries to do.
>
> 3. This can't protect against SIGKILL or SIGSTOP anyway. Another
> thread can do signal_wake_up(), say, do_signal_stop() or
> complete_signal() or debugger.
>
> 4. sigprocmask(SIG_BLOCK, allsigs) doesn't necessarily clears
> TIF_SIGPENDING, say, freezing() or ->jobctl.
>
> 5. device_write() looks equally wrong by the same reason.
>
> Looks like, this tries to protect some wait_event_interruptible() logic
> from signals, it should be turned into uninterruptible wait. Or we need
> to implement something like signals_stop/start for such a use-case.
I can't remember why that signal code exists, or if I ever knew; it was
there when the code was added seven years ago. I agree that if there's
something we cannot interrupt, we should use uninterruptible, but I don't
see any cases of that either. I think we should just remove it all
(untested):
From: David Teigland <teigl...@redhat.com>
Date: Mon, 12 Aug 2013 15:22:43 -0500
Subject: [PATCH] dlm: remove signal blocking
The signal blocking was incorrect and unnecessary
so just remove it.
Signed-off-by: David Teigland <teigl...@redhat.com>
---
fs/dlm/user.c | 25 ++++++-------------------
1 file changed, 6 insertions(+), 19 deletions(-)
diff --git a/fs/dlm/user.c b/fs/dlm/user.c
index 911649a..142e216 100644
--- a/fs/dlm/user.c
+++ b/fs/dlm/user.c
@@ -493,7 +493,6 @@ static ssize_t device_write(struct file *file, const char
__user *buf,
{
struct dlm_user_proc *proc = file->private_data;
struct dlm_write_request *kbuf;
- sigset_t tmpsig, allsigs;
int error;
#ifdef CONFIG_COMPAT
@@ -557,9 +556,6 @@ static ssize_t device_write(struct file *file, const char
__user *buf,
goto out_free;
}
- sigfillset(&allsigs);
- sigprocmask(SIG_BLOCK, &allsigs, &tmpsig);
-
error = -EINVAL;
switch (kbuf->cmd)
@@ -567,7 +563,7 @@ static ssize_t device_write(struct file *file, const char
__user *buf,
case DLM_USER_LOCK:
if (!proc) {
log_print("no locking on control device");
- goto out_sig;
+ goto out_free;
}
error = device_user_lock(proc, &kbuf->i.lock);
break;
@@ -575,7 +571,7 @@ static ssize_t device_write(struct file *file, const char
__user *buf,
case DLM_USER_UNLOCK:
if (!proc) {
log_print("no locking on control device");
- goto out_sig;
+ goto out_free;
}
error = device_user_unlock(proc, &kbuf->i.lock);
break;
@@ -583,7 +579,7 @@ static ssize_t device_write(struct file *file, const char
__user *buf,
case DLM_USER_DEADLOCK:
if (!proc) {
log_print("no locking on control device");
- goto out_sig;
+ goto out_free;
}
error = device_user_deadlock(proc, &kbuf->i.lock);
break;
@@ -591,7 +587,7 @@ static ssize_t device_write(struct file *file, const char
__user *buf,
case DLM_USER_CREATE_LOCKSPACE:
if (proc) {
log_print("create/remove only on control device");
- goto out_sig;
+ goto out_free;
}
error = device_create_lockspace(&kbuf->i.lspace);
break;
@@ -599,7 +595,7 @@ static ssize_t device_write(struct file *file, const char
__user *buf,
case DLM_USER_REMOVE_LOCKSPACE:
if (proc) {
log_print("create/remove only on control device");
- goto out_sig;
+ goto out_free;
}
error = device_remove_lockspace(&kbuf->i.lspace);
break;
@@ -607,7 +603,7 @@ static ssize_t device_write(struct file *file, const char
__user *buf,
case DLM_USER_PURGE:
if (!proc) {
log_print("no locking on control device");
- goto out_sig;
+ goto out_free;
}
error = device_user_purge(proc, &kbuf->i.purge);
break;
@@ -617,8 +613,6 @@ static ssize_t device_write(struct file *file, const char
__user *buf,
kbuf->cmd);
}
- out_sig:
- sigprocmask(SIG_SETMASK, &tmpsig, NULL);
out_free:
kfree(kbuf);
return error;
@@ -659,15 +653,11 @@ static int device_close(struct inode *inode, struct file
*file)
{
struct dlm_user_proc *proc = file->private_data;
struct dlm_ls *ls;
- sigset_t tmpsig, allsigs;
ls = dlm_find_lockspace_local(proc->lockspace);
if (!ls)
return -ENOENT;
- sigfillset(&allsigs);
- sigprocmask(SIG_BLOCK, &allsigs, &tmpsig);
-
set_bit(DLM_PROC_FLAGS_CLOSING, &proc->flags);
dlm_clear_proc_locks(ls, proc);
@@ -685,9 +675,6 @@ static int device_close(struct inode *inode, struct file
*file)
/* FIXME: AUTOFREE: if this ls is no longer used do
device_remove_lockspace() */
- sigprocmask(SIG_SETMASK, &tmpsig, NULL);
- recalc_sigpending();
-
return 0;
}
--
1.8.3.1
--
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/
