Hi,
On Thu, Jul 13, 2023 at 10:49 AM Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
>
> 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: stable@xxxxxxxxxxxxxxx
> > Signed-off-by: Alexander Aring <aahringo@xxxxxxxxxx>
>
> Why is adding a new uapi a stable patch?
>
because the user space is just to copy the flags back to the kernel. I
thought it would work. :)
> > ---
> > 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.
>
I will ignore handling this issue for older kernels because it would
probably be fine that the user space never gets an invalid value
handled.
> Also, don't wrap your strings, checkpatch should have told you this.
>
That is correct and I was ignoring it as the implementation has
another wrapped string somewhere else. It is a warning not an error.
Will send a v2 to not wrap the string around and drop Fixes and cc stable.
- Alex
