| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <PAXPR04MB908967BF3EE535F7F5C6377EE31D9@PAXPR04MB9089.eurprd04.prod.outlook.com>
Date: Mon, 28 Mar 2022 02:00:30 +0000
From: "S.J. Wang" <shengjiu.wang@....com>
To: Mathieu Poirier <mathieu.poirier@...aro.org>
CC: "bjorn.andersson@...aro.org" <bjorn.andersson@...aro.org>,
"linux-remoteproc@...r.kernel.org" <linux-remoteproc@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"shengjiu.wang@...il.com" <shengjiu.wang@...il.com>
Subject: Re: [PATCH] remoteproc: core: Remove state checking before changing
state
Hi
> > > >
> > > > There is no mutex protecting of these state checking, which can't
> > > > garantee there is no another instance is trying to do same operation.
> > > >
> > > > The reference counter rproc->power is used to manage state
> > > > changing and there is mutex protection in each operation function
> > > > for multi instance case.
> > > >
> > > > So remove this state checking in rproc_cdev_write() and
> > > > state_store(), just let reference counter rproc->power to manage the
> behaviors.
> > > >
> > > > Signed-off-by: Shengjiu Wang <shengjiu.wang@....com>
> > > > ---
> > > > drivers/remoteproc/remoteproc_cdev.c | 11 -----------
> > > > drivers/remoteproc/remoteproc_sysfs.c | 11 -----------
> > > > 2 files changed, 22 deletions(-)
> > > >
> > > > diff --git a/drivers/remoteproc/remoteproc_cdev.c
> > > > b/drivers/remoteproc/remoteproc_cdev.c
> > > > index 906ff3c4dfdd..687f205fd70a 100644
> > > > --- a/drivers/remoteproc/remoteproc_cdev.c
> > > > +++ b/drivers/remoteproc/remoteproc_cdev.c
> > > > @@ -32,21 +32,10 @@ static ssize_t rproc_cdev_write(struct file
> > > > *filp,
> > > const char __user *buf, size_
> > > > return -EFAULT;
> > > >
> > > > if (!strncmp(cmd, "start", len)) {
> > > > - if (rproc->state == RPROC_RUNNING ||
> > > > - rproc->state == RPROC_ATTACHED)
> > > > - return -EBUSY;
> > > > -
> > > > ret = rproc_boot(rproc);
> > > > } else if (!strncmp(cmd, "stop", len)) {
> > > > - if (rproc->state != RPROC_RUNNING &&
> > > > - rproc->state != RPROC_ATTACHED)
> > > > - return -EINVAL;
> > > > -
> > > > ret = rproc_shutdown(rproc);
> > > > } else if (!strncmp(cmd, "detach", len)) {
> > > > - if (rproc->state != RPROC_ATTACHED)
> > > > - return -EINVAL;
> > > > -
> > > > ret = rproc_detach(rproc);
> > > > } else {
> > > > dev_err(&rproc->dev, "Unrecognized option\n");
> > > > diff --git a/drivers/remoteproc/remoteproc_sysfs.c
> > > > b/drivers/remoteproc/remoteproc_sysfs.c
> > > > index 51a04bc6ba7a..8c7ea8922638 100644
> > > > --- a/drivers/remoteproc/remoteproc_sysfs.c
> > > > +++ b/drivers/remoteproc/remoteproc_sysfs.c
> > > > @@ -194,23 +194,12 @@ static ssize_t state_store(struct device *dev,
> > > > int ret = 0;
> > > >
> > > > if (sysfs_streq(buf, "start")) {
> > > > - if (rproc->state == RPROC_RUNNING ||
> > > > - rproc->state == RPROC_ATTACHED)
> > > > - return -EBUSY;
> > > > -
> > >
> > > As per my previous comment the above conditions need to be moved
> > > into rproc_boot().
> > >
> > > > ret = rproc_boot(rproc);
> > > > if (ret)
> > > > dev_err(&rproc->dev, "Boot failed: %d\n", ret);
> > > > } else if (sysfs_streq(buf, "stop")) {
> > > > - if (rproc->state != RPROC_RUNNING &&
> > > > - rproc->state != RPROC_ATTACHED)
> > > > - return -EINVAL;
> > > > -
> > > > ret = rproc_shutdown(rproc);
> > > > } else if (sysfs_streq(buf, "detach")) {
> > > > - if (rproc->state != RPROC_ATTACHED)
> > > > - return -EINVAL;
> > > > -
> > >
> > > This patch should have been part of a patch series with your other
> > > work sent on March 18th[1].
> > >
> > > Thanks,
> > > Mathieu
> > >
> > > [1]. [PATCH] remoteproc: core: check rproc->power value before
> > > decreasing it
> > >
> >
> > Thanks for the comments.
> > I still have one question, if there are two instances independently to 'start'
> > 'stop' remoteproc, for example:
> >
> > Instance1: echo start > /sys/class/remoteproc/remoteproc0/state
> > Instance2: echo start > /sys/class/remoteproc/remoteproc0/state
> >
> > ...
> >
> > Instance2: echo stop > /sys/class/remoteproc/remoteproc0/state
> > ...
> > Instance1: echo stop > /sys/class/remoteproc/remoteproc0/state
> >
> > When instance2 'stop' the remoteproc, then instance1 will be impacted
> > for It still needs the service from remoteproc.
> >
> > That's why I just removed of the checking state, didn't move them to
> > rproc_boot()/rproc_shutdown()/rproc_detach(). And in order to utilize
> > the reference counter (rproc->power) to handle the multi-instance case.
>
> Thanks for the exta information, now I understand the problem. The above
> should be part of the changelog.
>
> There are two problems here:
>
> 1) Dealing with race conditions when checking the state of a remote
> processor.
> 2) Properly dealing with the remote processor's reference count.
>
> For the first one, state checks control the remote processor state machine
> and can't simply be removed. They have to be brought inside the mutex lock
> in order to avoid race conditions when multiple users interact with the
> remote processor.
>
> For the second one, moving the rproc->state checks inside the mutex lock in
> rproc_shutdown() and rproc_detach() will work with the rproc->power check.
>
> The problem is with rproc_boot(). For (at least) two years now we have lost
> the capability to increase the rproc->power reference count from sysfs and
> the cdev interface due to the rproc-state checks in state_store() and
> rproc_cdev_write(). I think that should be corrected but it will introduce a
> user space visible change, which needs to be treated carefully.
>
> With the above in mind, please send a patchset that includes one patch that
> removes the rproc->state checks from the "start" command in state_store()
> and rproc_cdev_write(). The other patch should move the rproc->state
> checks for the "stop" and "detach" command inside rproc_shutdown() and
> rproc_detach().
>
> With that we can start a discussion on the best way to proceed.
>
Ok, thanks, will send the patches.
Best regards
Wang Shengjiu
Powered by blists - more mailing lists