On Thu, Jul 13, 2023 at 10:40:28AM -0400, Alexander Aring wrote:
> This patch introduces a new flag DLM_PLOCK_FL_NO_REPLY in case an dlm
> plock operation should not send a reply back. Currently this is kind of
> being handled in DLM_PLOCK_FL_CLOSE, but DLM_PLOCK_FL_CLOSE has more
> meanings that it will remove all waiters for a specific nodeid/owner
> values in by doing a unlock operation. In case of an error in dlm user
> space software e.g. dlm_controld we get an reply with an error back.
> This cannot be matched because there is no op to match in recv_list. We
> filter now on DLM_PLOCK_FL_NO_REPLY in case we had an error back as
> reply. In newer dlm_controld version it will never send a result back
> when DLM_PLOCK_FL_NO_REPLY is set. This filter is a workaround to handle
> older dlm_controld versions.
>
> Fixes: 901025d2f319 ("dlm: make plock operation killable")
> Cc: sta...@vger.kernel.org
> Signed-off-by: Alexander Aring <aahri...@redhat.com>
Why is adding a new uapi a stable patch?
> ---
> fs/dlm/plock.c | 23 +++++++++++++++++++----
> include/uapi/linux/dlm_plock.h | 1 +
> 2 files changed, 20 insertions(+), 4 deletions(-)
>
> diff --git a/fs/dlm/plock.c b/fs/dlm/plock.c
> index 70a4752ed913..7fe9f4b922d3 100644
> --- a/fs/dlm/plock.c
> +++ b/fs/dlm/plock.c
> @@ -96,7 +96,7 @@ static void do_unlock_close(const struct dlm_plock_info
> *info)
> op->info.end = OFFSET_MAX;
> op->info.owner = info->owner;
>
> - op->info.flags |= DLM_PLOCK_FL_CLOSE;
> + op->info.flags |= (DLM_PLOCK_FL_CLOSE | DLM_PLOCK_FL_NO_REPLY);
> send_op(op);
> }
>
> @@ -293,7 +293,7 @@ int dlm_posix_unlock(dlm_lockspace_t *lockspace, u64
> number, struct file *file,
> op->info.owner = (__u64)(long) fl->fl_owner;
>
> if (fl->fl_flags & FL_CLOSE) {
> - op->info.flags |= DLM_PLOCK_FL_CLOSE;
> + op->info.flags |= (DLM_PLOCK_FL_CLOSE | DLM_PLOCK_FL_NO_REPLY);
> send_op(op);
> rv = 0;
> goto out;
> @@ -392,7 +392,7 @@ static ssize_t dev_read(struct file *file, char __user
> *u, size_t count,
> spin_lock(&ops_lock);
> if (!list_empty(&send_list)) {
> op = list_first_entry(&send_list, struct plock_op, list);
> - if (op->info.flags & DLM_PLOCK_FL_CLOSE)
> + if (op->info.flags & DLM_PLOCK_FL_NO_REPLY)
> list_del(&op->list);
> else
> list_move_tail(&op->list, &recv_list);
> @@ -407,7 +407,7 @@ static ssize_t dev_read(struct file *file, char __user
> *u, size_t count,
> that were generated by the vfs cleaning up for a close
> (the process did not make an unlock call). */
>
> - if (op->info.flags & DLM_PLOCK_FL_CLOSE)
> + if (op->info.flags & DLM_PLOCK_FL_NO_REPLY)
> dlm_release_plock_op(op);
>
> if (copy_to_user(u, &info, sizeof(info)))
> @@ -433,6 +433,21 @@ static ssize_t dev_write(struct file *file, const char
> __user *u, size_t count,
> if (check_version(&info))
> return -EINVAL;
>
> + /* Some old dlm user space software will send replies back,
> + * even if DLM_PLOCK_FL_NO_REPLY is set (because the flag is
> + * new) e.g. if a error occur. We can't match them in recv_list
> + * because they were never be part of it. We filter it here,
> + * new dlm user space software will filter it in user space.
> + *
> + * In future this handling can be removed.
> + */
> + if (info.flags & DLM_PLOCK_FL_NO_REPLY) {
> + pr_info("Received unexpected reply from op %d, "
> + "please update DLM user space software!\n",
> + info.optype);
Never allow userspace to spam the kernel log. And this is not going to
work, you need to handle the error and at most, report this to userspace
once.
Also, don't wrap your strings, checkpatch should have told you this.
thanks,
greg k-h
