Re: [PATCH] ACPI: Make custom_method use per-open state

[Sebastian Grzywna]

Dnia 2023-02-01, o godz. 19:34:48
"Rafael J. Wysocki" <rafael@xxxxxxxxxx> napisał(a):

> On Wed, Feb 1, 2023 at 12:38 AM Pedro Falcato
> <pedro.falcato@xxxxxxxxx> wrote:
> >
> > Make custom_method keep its own per-file-open state instead of
> > global state in order to avoid race conditions[1] and other
> > possible conflicts with other concurrent users.
> >
> > Link:
> > https://lore.kernel.org/linux-acpi/20221227063335.61474-1-zh.nvgt@xxxxxxxxx/
> > # [1] Reported-by: Hang Zhang <zh.nvgt@xxxxxxxxx> Cc: Swift Geek
> > <swiftgeek@xxxxxxxxx> Signed-off-by: Pedro Falcato
> > <pedro.falcato@xxxxxxxxx> ---
> >  This patch addresses Hang's problems plus the ones raised by
> > Rafael in his review (see link above).
> > https://lore.kernel.org/lkml/2667007.mvXUDI8C0e@kreacher/ was
> > submitted but since there were still people that wanted this
> > feature, I took my time to write up a patch that should fix the
> > issues. Hopefully the linux-acpi maintainers have not decided to
> > remove custom_method just yet.  
> 
> Well, thanks for the patch, but yes, they have.  Sorry.
 
Hi Rafael,
Can you please explain why you don't want to keep it, given there's a
patch? I find it really useful in my day-to-day as a firmware engineer.
I don't see much happening in git history of
drivers/acpi/custom_method.c , and I don't see anything that was
specifically changed in it in past 10 years to keep it being
functional. Without your more detailed explanation I have hard time
understanding your decision to remove it, since I'm not a kernel
developer myself.
Thanks,
Sebastian Grzywna

> >  drivers/acpi/custom_method.c | 119
> > +++++++++++++++++++++++++++-------- 1 file changed, 92
> > insertions(+), 27 deletions(-)
> >
> > diff --git a/drivers/acpi/custom_method.c
> > b/drivers/acpi/custom_method.c index d39a9b47472..034fb14f118 100644
> > --- a/drivers/acpi/custom_method.c
> > +++ b/drivers/acpi/custom_method.c
> > @@ -17,73 +17,138 @@ MODULE_LICENSE("GPL");
> >
> >  static struct dentry *cm_dentry;
> >
> > +struct custom_method_state {
> > +       char *buf;
> > +       u32 max_size;
> > +       u32 uncopied_bytes;
> > +       struct mutex lock;
> > +};
> > +
> > +static int cm_open(struct inode *inode, struct file *file)
> > +{
> > +       struct custom_method_state *state;
> > +
> > +       state = kzalloc(sizeof(struct custom_method_state),
> > GFP_KERNEL); +
> > +       if (!state)
> > +               return -ENOMEM;
> > +
> > +       file->private_data = state;
> > +       mutex_init(&state->lock);
> > +
> > +       return 0;
> > +}
> > +
> > +static int cm_release(struct inode *inode, struct file *file)
> > +{
> > +       struct custom_method_state *state;
> > +
> > +       state = file->private_data;
> > +
> > +       mutex_destroy(&state->lock);
> > +
> > +       /* Make sure the buf gets freed */
> > +       kfree(state->buf);
> > +
> > +       kfree(state);
> > +       return 0;
> > +}
> > +
> >  /* /sys/kernel/debug/acpi/custom_method */
> >
> >  static ssize_t cm_write(struct file *file, const char __user
> > *user_buf, size_t count, loff_t *ppos)
> >  {
> > -       static char *buf;
> > -       static u32 max_size;
> > -       static u32 uncopied_bytes;
> > +       struct custom_method_state *state;
> > +       char *buf;
> >
> >         struct acpi_table_header table;
> >         acpi_status status;
> >         int ret;
> >
> > +       state = file->private_data;
> > +       buf = state->buf;
> > +
> >         ret = security_locked_down(LOCKDOWN_ACPI_TABLES);
> >         if (ret)
> >                 return ret;
> >
> > +       mutex_lock(&state->lock);
> > +
> >         if (!(*ppos)) {
> >                 /* parse the table header to get the table length */
> > -               if (count <= sizeof(struct acpi_table_header))
> > -                       return -EINVAL;
> > +               if (count <= sizeof(struct acpi_table_header)) {
> > +                       count = -EINVAL;
> > +                       goto out;
> > +               }
> > +
> >                 if (copy_from_user(&table, user_buf,
> > -                                  sizeof(struct
> > acpi_table_header)))
> > -                       return -EFAULT;
> > -               uncopied_bytes = max_size = table.length;
> > +                                  sizeof(struct
> > acpi_table_header))) {
> > +                       count = -EFAULT;
> > +                       goto out;
> > +               }
> > +
> > +               state->uncopied_bytes = state->max_size =
> > table.length; /* make sure the buf is not allocated */
> >                 kfree(buf);
> > -               buf = kzalloc(max_size, GFP_KERNEL);
> > -               if (!buf)
> > -                       return -ENOMEM;
> > +               buf = state->buf = kzalloc(state->max_size,
> > GFP_KERNEL);
> > +               if (!buf) {
> > +                       count = -ENOMEM;
> > +                       goto out;
> > +               }
> >         }
> >
> > -       if (buf == NULL)
> > -               return -EINVAL;
> > +       /* Check if someone seeked ahead or if we errored out
> > +        * (buf will be NULL)
> > +        */
> > +       if (buf == NULL) {
> > +               count = -EINVAL;
> > +               goto out;
> > +       }
> >
> > -       if ((*ppos > max_size) ||
> > -           (*ppos + count > max_size) ||
> > +       if ((*ppos > state->max_size) ||
> > +           (*ppos + count > state->max_size) ||
> >             (*ppos + count < count) ||
> > -           (count > uncopied_bytes)) {
> > -               kfree(buf);
> > -               buf = NULL;
> > -               return -EINVAL;
> > +           (count > state->uncopied_bytes)) {
> > +               count = -EINVAL;
> > +               goto err_free;
> >         }
> >
> >         if (copy_from_user(buf + (*ppos), user_buf, count)) {
> > -               kfree(buf);
> > -               buf = NULL;
> > -               return -EFAULT;
> > +               count = -EFAULT;
> > +               goto err_free;
> >         }
> >
> > -       uncopied_bytes -= count;
> > +       state->uncopied_bytes -= count;
> >         *ppos += count;
> >
> > -       if (!uncopied_bytes) {
> > +       if (!state->uncopied_bytes) {
> >                 status = acpi_install_method(buf);
> >                 kfree(buf);
> > -               buf = NULL;
> > -               if (ACPI_FAILURE(status))
> > -                       return -EINVAL;
> > +               state->buf = NULL;
> > +
> > +               if (ACPI_FAILURE(status)) {
> > +                       count = -EINVAL;
> > +                       goto out;
> > +               }
> > +
> >                 add_taint(TAINT_OVERRIDDEN_ACPI_TABLE,
> > LOCKDEP_NOW_UNRELIABLE); }
> >
> > +out:
> > +       mutex_unlock(&state->lock);
> > +       return count;
> > +err_free:
> > +       mutex_unlock(&state->lock);
> > +       kfree(buf);
> > +       state->buf = NULL;
> >         return count;
> >  }
> >
> >  static const struct file_operations cm_fops = {
> >         .write = cm_write,
> > +       .open = cm_open,
> > +       .release = cm_release,
> >         .llseek = default_llseek,
> >  };
> >
> > --
> > 2.39.0
> >