| Message ID | 20230131233755.58942-1-pedro.falcato@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | ACPI: Make custom_method use per-open state | expand |
On Wed, Feb 1, 2023 at 12:38 AM Pedro Falcato <pedro.falcato@gmail.com> 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@gmail.com/ # [1] > Reported-by: Hang Zhang <zh.nvgt@gmail.com> > Cc: Swift Geek <swiftgeek@gmail.com> > Signed-off-by: Pedro Falcato <pedro.falcato@gmail.com> > --- > 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. > 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 >
Dnia 2023-02-01, o godz. 19:34:48 "Rafael J. Wysocki" <rafael@kernel.org> napisał(a): > On Wed, Feb 1, 2023 at 12:38 AM Pedro Falcato > <pedro.falcato@gmail.com> 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@gmail.com/ > > # [1] Reported-by: Hang Zhang <zh.nvgt@gmail.com> Cc: Swift Geek > > <swiftgeek@gmail.com> Signed-off-by: Pedro Falcato > > <pedro.falcato@gmail.com> --- > > 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 > >
On Thu, Feb 2, 2023 at 2:04 AM Marty E. Plummer <hanetzer@startmail.com> wrote: > > From: "Rafael J. Wysocki" <rafael@kernel.org> > > > On Wed, Feb 1, 2023 at 12:38 AM Pedro Falcato <pedro.falcato@gmail.com> 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@gmail.com/ # [1] > > > Reported-by: Hang Zhang <zh.nvgt@gmail.com> > > > Cc: Swift Geek <swiftgeek@gmail.com> > > > Signed-off-by: Pedro Falcato <pedro.falcato@gmail.com> > > > --- > > > 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. > > Honestly, I think that's a bit of a cop out. This is git. git revert exists, > and you're crippling the abilities of quite a lot of coreboot/etc development. Perhaps they can try to use the ACPI debugger that's in the kernel now instead of a known-broken interface?
On Thu, Feb 2, 2023 at 8:50 AM Sebastian Grzywna <swiftgeek@gmail.com> wrote: > > Dnia 2023-02-01, o godz. 19:34:48 > "Rafael J. Wysocki" <rafael@kernel.org> napisał(a): > > > On Wed, Feb 1, 2023 at 12:38 AM Pedro Falcato > > <pedro.falcato@gmail.com> 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@gmail.com/ > > > # [1] Reported-by: Hang Zhang <zh.nvgt@gmail.com> Cc: Swift Geek > > > <swiftgeek@gmail.com> Signed-off-by: Pedro Falcato > > > <pedro.falcato@gmail.com> --- > > > 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? Because this interface was a bad idea to start with and its implementation is questionable at the design level. Granted, at the time it was introduced, there was no alternative, but there is the AML debugger in the kernel now and as far as debugging is concerned, it is actually more powerful than custom_metod AFAICS. See Documentation/firmware-guide/acpi/aml-debugger.rst. If the AML debugger has problems, I would very much prefer fixing them to the perpetual maintenance of custom_method. > 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. It's been always conceptually questionable, problematic from the security standpoint and implemented poorly. Also its documentation is outdated. The patches fixing its most apparent functional issues don't actually address much of the above. The AML debugger should really be used for debug rather than custom_method and honestly, what's the purpose of it beyond debug?
On Thu, Feb 2, 2023 at 11:03 AM Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Thu, Feb 2, 2023 at 8:50 AM Sebastian Grzywna <swiftgeek@gmail.com> wrote: > > > > Dnia 2023-02-01, o godz. 19:34:48 > > "Rafael J. Wysocki" <rafael@kernel.org> napisał(a): > > > > > On Wed, Feb 1, 2023 at 12:38 AM Pedro Falcato > > > <pedro.falcato@gmail.com> 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@gmail.com/ > > > > # [1] Reported-by: Hang Zhang <zh.nvgt@gmail.com> Cc: Swift Geek > > > > <swiftgeek@gmail.com> Signed-off-by: Pedro Falcato > > > > <pedro.falcato@gmail.com> --- > > > > 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? > > Because this interface was a bad idea to start with and its > implementation is questionable at the design level. > > Granted, at the time it was introduced, there was no alternative, but > there is the AML debugger in the kernel now and as far as debugging is > concerned, it is actually more powerful than custom_metod AFAICS. See > Documentation/firmware-guide/acpi/aml-debugger.rst. > > If the AML debugger has problems, I would very much prefer fixing them > to the perpetual maintenance of custom_method. > > > 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. > > It's been always conceptually questionable, problematic from the > security standpoint and implemented poorly. Also its documentation is > outdated. > > The patches fixing its most apparent functional issues don't actually > address much of the above. > > The AML debugger should really be used for debug rather than > custom_method and honestly, what's the purpose of it beyond debug? The above said, if people really do care about custom_method, it can be retained, but its documentation needs to be updated to cover the current requirements (according to Rui, they have changed after some upstream ACPICA changes). Also note that the upstream ACPICA may not be guaranteed to avoid breaking this interface in the future, as it depends on acpi_install_method() that is provided by ACPICA specifically for the use in the AML debugger.
On Thu, Feb 2, 2023 at 10:45 AM Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Thu, Feb 2, 2023 at 11:03 AM Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > On Thu, Feb 2, 2023 at 8:50 AM Sebastian Grzywna <swiftgeek@gmail.com> wrote: > > > > > > Dnia 2023-02-01, o godz. 19:34:48 > > > "Rafael J. Wysocki" <rafael@kernel.org> napisał(a): > > > > > > > On Wed, Feb 1, 2023 at 12:38 AM Pedro Falcato > > > > <pedro.falcato@gmail.com> 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@gmail.com/ > > > > > # [1] Reported-by: Hang Zhang <zh.nvgt@gmail.com> Cc: Swift Geek > > > > > <swiftgeek@gmail.com> Signed-off-by: Pedro Falcato > > > > > <pedro.falcato@gmail.com> --- > > > > > 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? > > > > Because this interface was a bad idea to start with and its > > implementation is questionable at the design level. > > > > Granted, at the time it was introduced, there was no alternative, but > > there is the AML debugger in the kernel now and as far as debugging is > > concerned, it is actually more powerful than custom_metod AFAICS. See > > Documentation/firmware-guide/acpi/aml-debugger.rst. > > > > If the AML debugger has problems, I would very much prefer fixing them > > to the perpetual maintenance of custom_method. > > > > > 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. > > > > It's been always conceptually questionable, problematic from the > > security standpoint and implemented poorly. Also its documentation is > > outdated. > > > > The patches fixing its most apparent functional issues don't actually > > address much of the above. > > > > The AML debugger should really be used for debug rather than > > custom_method and honestly, what's the purpose of it beyond debug? > > The above said, if people really do care about custom_method, it can > be retained, but its documentation needs to be updated to cover the > current requirements (according to Rui, they have changed after some > upstream ACPICA changes). > > Also note that the upstream ACPICA may not be guaranteed to avoid > breaking this interface in the future, as it depends on > acpi_install_method() that is provided by ACPICA specifically for the > use in the AML debugger. Just to be clear, I have no stake in this matter and wrote this patch because someone complained on #coreboot about the removal. This patch was mostly trivial to write and if you decide there really is no value in keeping this, I'm a-OK.
On Thu, 2 Feb 2023 11:03:47 +0100 "Rafael J. Wysocki" <rafael@kernel.org> wrote: > On Thu, Feb 2, 2023 at 8:50 AM Sebastian Grzywna > <swiftgeek@gmail.com> wrote: > > > > Dnia 2023-02-01, o godz. 19:34:48 > > "Rafael J. Wysocki" <rafael@kernel.org> napisał(a): > > > > > On Wed, Feb 1, 2023 at 12:38 AM Pedro Falcato > > > <pedro.falcato@gmail.com> 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@gmail.com/ > > > > # [1] Reported-by: Hang Zhang <zh.nvgt@gmail.com> Cc: Swift Geek > > > > <swiftgeek@gmail.com> Signed-off-by: Pedro Falcato > > > > <pedro.falcato@gmail.com> --- > > > > 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? > > Because this interface was a bad idea to start with and its > implementation is questionable at the design level. > > Granted, at the time it was introduced, there was no alternative, but > there is the AML debugger in the kernel now and as far as debugging is > concerned, it is actually more powerful than custom_metod AFAICS. See > Documentation/firmware-guide/acpi/aml-debugger.rst. > > If the AML debugger has problems, I would very much prefer fixing them > to the perpetual maintenance of custom_method. > > > 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. > > It's been always conceptually questionable, problematic from the > security standpoint and implemented poorly. Also its documentation is > outdated. > > The patches fixing its most apparent functional issues don't actually > address much of the above. > > The AML debugger should really be used for debug rather than > custom_method and honestly, what's the purpose of it beyond debug? I tried to investigate how to get my workflow going with tools/power/acpi/acpidbg , and after looking at drivers/acpi/acpica/dbinput.c I am a bit confused as to how should I enable CMD_LOAD properly, and if it's not in Kconfig then how supported/tested is this feature. acpica-reference_19.pdf is also confusing me a bit with mention of "Unloading the DSDT is not allowed", making me worry that live reloading of DSDT would not be possible (which would be superset of overriding just a single method). I want to be able to swap a particular method, and have it run as close to normally as possible, sometimes for prolonged period of time, but most of the time I will fail at writing a patch and will need to swap it again with another attempted fix.
On Thu, Feb 2, 2023 at 3:48 PM Pedro Falcato <pedro.falcato@gmail.com> wrote: > > On Thu, Feb 2, 2023 at 10:45 AM Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > On Thu, Feb 2, 2023 at 11:03 AM Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > > > On Thu, Feb 2, 2023 at 8:50 AM Sebastian Grzywna <swiftgeek@gmail.com> wrote: > > > > > > > > Dnia 2023-02-01, o godz. 19:34:48 > > > > "Rafael J. Wysocki" <rafael@kernel.org> napisał(a): > > > > > > > > > On Wed, Feb 1, 2023 at 12:38 AM Pedro Falcato > > > > > <pedro.falcato@gmail.com> 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@gmail.com/ > > > > > > # [1] Reported-by: Hang Zhang <zh.nvgt@gmail.com> Cc: Swift Geek > > > > > > <swiftgeek@gmail.com> Signed-off-by: Pedro Falcato > > > > > > <pedro.falcato@gmail.com> --- > > > > > > 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? > > > > > > Because this interface was a bad idea to start with and its > > > implementation is questionable at the design level. > > > > > > Granted, at the time it was introduced, there was no alternative, but > > > there is the AML debugger in the kernel now and as far as debugging is > > > concerned, it is actually more powerful than custom_metod AFAICS. See > > > Documentation/firmware-guide/acpi/aml-debugger.rst. > > > > > > If the AML debugger has problems, I would very much prefer fixing them > > > to the perpetual maintenance of custom_method. > > > > > > > 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. > > > > > > It's been always conceptually questionable, problematic from the > > > security standpoint and implemented poorly. Also its documentation is > > > outdated. > > > > > > The patches fixing its most apparent functional issues don't actually > > > address much of the above. > > > > > > The AML debugger should really be used for debug rather than > > > custom_method and honestly, what's the purpose of it beyond debug? > > > > The above said, if people really do care about custom_method, it can > > be retained, but its documentation needs to be updated to cover the > > current requirements (according to Rui, they have changed after some > > upstream ACPICA changes). > > > > Also note that the upstream ACPICA may not be guaranteed to avoid > > breaking this interface in the future, as it depends on > > acpi_install_method() that is provided by ACPICA specifically for the > > use in the AML debugger. > > Just to be clear, I have no stake in this matter and wrote this patch because > someone complained on #coreboot about the removal. This patch was mostly > trivial to write and if you decide there really is no value in keeping > this, I'm a-OK. > > From a maintainers' PoV, if the AML debugger completely encompasses this > from a functional PoV with a good UX, I would probably encourage you to > remove this once and for all (what's the value in having 2 of the same > functionality?). > > Hopefully the FW folks can give more context and feedback. > > -- > Pedro Hi Rafael and other stakeholders, Gentle ping. Are we axing the feature? Are we reviewing/merging this patch? Having a straight up racy/broken feature in the kernel (even if only under debugfs) is not ideal.
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, };
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@gmail.com/ # [1] Reported-by: Hang Zhang <zh.nvgt@gmail.com> Cc: Swift Geek <swiftgeek@gmail.com> Signed-off-by: Pedro Falcato <pedro.falcato@gmail.com> --- 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. drivers/acpi/custom_method.c | 119 +++++++++++++++++++++++++++-------- 1 file changed, 92 insertions(+), 27 deletions(-)