From: Sebastian Grzywna <swiftgeek@gmail.com>
To: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Pedro Falcato <pedro.falcato@gmail.com>,
linux-acpi@vger.kernel.org, Len Brown <lenb@kernel.org>,
linux-kernel@vger.kernel.org, rui.zhang@intel.com,
Hang Zhang <zh.nvgt@gmail.com>
Subject: Re: [PATCH] ACPI: Make custom_method use per-open state
Date: Thu, 2 Feb 2023 08:49:53 +0100 [thread overview]
Message-ID: <20230202084953.3035c6e3@gmail.com> (raw)
In-Reply-To: <CAJZ5v0iXcRFamA+mE837=zHReBT-+8WmMeRDR7L9R+FVpLr25A@mail.gmail.com>
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
> >
next prev parent reply other threads:[~2023-02-02 7:50 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-01-31 23:37 [PATCH] ACPI: Make custom_method use per-open state Pedro Falcato
2023-02-01 18:34 ` Rafael J. Wysocki
2023-02-02 7:49 ` Sebastian Grzywna [this message]
2023-02-02 10:03 ` Rafael J. Wysocki
2023-02-02 10:44 ` Rafael J. Wysocki
2023-02-02 15:48 ` Pedro Falcato
2023-03-13 23:45 ` Pedro Falcato
2023-02-03 6:42 ` Sebastian Grzywna
2023-02-02 9:34 ` Rafael J. Wysocki
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20230202084953.3035c6e3@gmail.com \
--to=swiftgeek@gmail.com \
--cc=lenb@kernel.org \
--cc=linux-acpi@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=pedro.falcato@gmail.com \
--cc=rafael@kernel.org \
--cc=rui.zhang@intel.com \
--cc=zh.nvgt@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).