linux-kernel - Re: [PATCH v4 3/5] remoteproc: Pass type of shutdown to subdev remove

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20171205064611.GM28761@minitux>
Date:   Mon, 4 Dec 2017 22:46:11 -0800
From:   Bjorn Andersson <bjorn.andersson@...aro.org>
To:     Arnaud Pouliquen <arnaud.pouliquen@...com>
Cc:     Andy Gross <andy.gross@...aro.org>,
        Ohad Ben-Cohen <ohad@...ery.com>,
        Arun Kumar Neelakantam <aneela@...eaurora.org>,
        Chris Lew <clew@...eaurora.org>, linux-kernel@...r.kernel.org,
        linux-arm-msm@...r.kernel.org, linux-soc@...r.kernel.org,
        linux-remoteproc@...r.kernel.org
Subject: Re: [PATCH v4 3/5] remoteproc: Pass type of shutdown to subdev remove

On Fri 01 Dec 06:50 PST 2017, Arnaud Pouliquen wrote:

> hello Bjorn,
> 
> Sorry for these late remarks/questions
> 

No worries, I'm happy to see you reading the patch!

> 
> On 11/30/2017 02:16 AM, Bjorn Andersson wrote:
[..]
> > diff --git a/drivers/remoteproc/qcom_common.c b/drivers/remoteproc/qcom_common.c
[..]
> > @@ -785,17 +785,17 @@ static int rproc_probe_subdevices(struct rproc *rproc)
> >  
> >  unroll_registration:
> >  	list_for_each_entry_continue_reverse(subdev, &rproc->subdevs, node)
> > -		subdev->remove(subdev);
> > +		subdev->remove(subdev, false);
> Why do you need to do a non graceful remove in this case? This could
> lead to side effect like memory leakage...
> 

Regardless of this being true or false resources should always be
reclaimed.

The reason for introducing this is that the modem in the Qualcomm
platforms implements persistent storage and it's preferred to tell it to
flush the latest data to the storage server (on the Linux side) before
pulling the plug. But in the case of a firmware crash this mechanism
will not be operational and there's no point in attempting this
"graceful shutdown".

[..]
> > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> > index 44e630eb3d94..20a9467744ea 100644
> > --- a/include/linux/remoteproc.h
> > +++ b/include/linux/remoteproc.h
> > @@ -456,7 +456,7 @@ struct rproc_subdev {
> >  	struct list_head node;
> >  
> >  	int (*probe)(struct rproc_subdev *subdev);
> > -	void (*remove)(struct rproc_subdev *subdev);
> > +	void (*remove)(struct rproc_subdev *subdev, bool graceful);
> What about adding a new ops instead of a parameter, like a recovery
> callback?
> 

I think that for symmetry purposes it should be probe/remove in both
code paths. A possible alternative to the proposal would be to introduce
an operation "request_shutdown()" the would be called in the proposed
graceful code path.

However, in the Qualcomm SMD and GLINK (conceptually equivalent to
virtio-rpmsg) it is possible to open and close communication channels
and it's conceivable to see that the graceful case would close all
channels cleanly while the non-graceful case would just remove the rpmsg
devices (and leave the channel states/memory as is).

In this case a "request_shutdown()" would complicate things, compared to
the boolean.

Regards,
Bjorn