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 > >
