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/[email protected]/ # [1]
Reported-by: Hang Zhang <[email protected]>
Cc: Swift Geek <[email protected]>
Signed-off-by: Pedro Falcato <[email protected]>
---
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(-)
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 Wed, Feb 1, 2023 at 12:38 AM Pedro Falcato <[email protected]> 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/[email protected]/ # [1]
> Reported-by: Hang Zhang <[email protected]>
> Cc: Swift Geek <[email protected]>
> Signed-off-by: Pedro Falcato <[email protected]>
> ---
> 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" <[email protected]> napisał(a):
> On Wed, Feb 1, 2023 at 12:38 AM Pedro Falcato
> <[email protected]> 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/[email protected]/
> > # [1] Reported-by: Hang Zhang <[email protected]> Cc: Swift Geek
> > <[email protected]> Signed-off-by: Pedro Falcato
> > <[email protected]> ---
> > 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 <[email protected]> wrote:
>
> From: "Rafael J. Wysocki" <[email protected]>
>
> > On Wed, Feb 1, 2023 at 12:38 AM Pedro Falcato <[email protected]> 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/[email protected]/ # [1]
> > > Reported-by: Hang Zhang <[email protected]>
> > > Cc: Swift Geek <[email protected]>
> > > Signed-off-by: Pedro Falcato <[email protected]>
> > > ---
> > > 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 <[email protected]> wrote:
>
> Dnia 2023-02-01, o godz. 19:34:48
> "Rafael J. Wysocki" <[email protected]> napisał(a):
>
> > On Wed, Feb 1, 2023 at 12:38 AM Pedro Falcato
> > <[email protected]> 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/[email protected]/
> > > # [1] Reported-by: Hang Zhang <[email protected]> Cc: Swift Geek
> > > <[email protected]> Signed-off-by: Pedro Falcato
> > > <[email protected]> ---
> > > 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 <[email protected]> wrote:
>
> On Thu, Feb 2, 2023 at 8:50 AM Sebastian Grzywna <[email protected]> wrote:
> >
> > Dnia 2023-02-01, o godz. 19:34:48
> > "Rafael J. Wysocki" <[email protected]> napisał(a):
> >
> > > On Wed, Feb 1, 2023 at 12:38 AM Pedro Falcato
> > > <[email protected]> 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/[email protected]/
> > > > # [1] Reported-by: Hang Zhang <[email protected]> Cc: Swift Geek
> > > > <[email protected]> Signed-off-by: Pedro Falcato
> > > > <[email protected]> ---
> > > > 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 <[email protected]> wrote:
>
> On Thu, Feb 2, 2023 at 11:03 AM Rafael J. Wysocki <[email protected]> wrote:
> >
> > On Thu, Feb 2, 2023 at 8:50 AM Sebastian Grzywna <[email protected]> wrote:
> > >
> > > Dnia 2023-02-01, o godz. 19:34:48
> > > "Rafael J. Wysocki" <[email protected]> napisał(a):
> > >
> > > > On Wed, Feb 1, 2023 at 12:38 AM Pedro Falcato
> > > > <[email protected]> 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/[email protected]/
> > > > > # [1] Reported-by: Hang Zhang <[email protected]> Cc: Swift Geek
> > > > > <[email protected]> Signed-off-by: Pedro Falcato
> > > > > <[email protected]> ---
> > > > > 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
On Thu, 2 Feb 2023 11:03:47 +0100
"Rafael J. Wysocki" <[email protected]> wrote:
> On Thu, Feb 2, 2023 at 8:50 AM Sebastian Grzywna
> <[email protected]> wrote:
> >
> > Dnia 2023-02-01, o godz. 19:34:48
> > "Rafael J. Wysocki" <[email protected]> napisał(a):
> >
> > > On Wed, Feb 1, 2023 at 12:38 AM Pedro Falcato
> > > <[email protected]> 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/[email protected]/
> > > > # [1] Reported-by: Hang Zhang <[email protected]> Cc: Swift Geek
> > > > <[email protected]> Signed-off-by: Pedro Falcato
> > > > <[email protected]> ---
> > > > 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 <[email protected]> wrote:
>
> On Thu, Feb 2, 2023 at 10:45 AM Rafael J. Wysocki <[email protected]> wrote:
> >
> > On Thu, Feb 2, 2023 at 11:03 AM Rafael J. Wysocki <[email protected]> wrote:
> > >
> > > On Thu, Feb 2, 2023 at 8:50 AM Sebastian Grzywna <[email protected]> wrote:
> > > >
> > > > Dnia 2023-02-01, o godz. 19:34:48
> > > > "Rafael J. Wysocki" <[email protected]> napisał(a):
> > > >
> > > > > On Wed, Feb 1, 2023 at 12:38 AM Pedro Falcato
> > > > > <[email protected]> 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/[email protected]/
> > > > > > # [1] Reported-by: Hang Zhang <[email protected]> Cc: Swift Geek
> > > > > > <[email protected]> Signed-off-by: Pedro Falcato
> > > > > > <[email protected]> ---
> > > > > > 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.
--
Pedro