Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <cb388d8ea15b2c80a072dec74d9ededecb183a08.camel@gmail.com>
Subject: Re: [PATCH 2/2] scsi: ufs: Protect PM ops and err_handler from user
 access through sysfs
From:   Bean Huo <huobean@gmail.com>
To:     Can Guo <cang@codeaurora.org>
Cc:     asutoshd@codeaurora.org, nguyenb@codeaurora.org,
        hongwus@codeaurora.org, ziqichen@codeaurora.org,
        rnayak@codeaurora.org, linux-scsi@vger.kernel.org,
        kernel-team@android.com, saravanak@google.com, salyzyn@google.com,
        rjw@rjwysocki.net, Alim Akhtar <alim.akhtar@samsung.com>,
        Avri Altman <avri.altman@wdc.com>,
        "James E.J. Bottomley" <jejb@linux.ibm.com>,
        "Martin K. Petersen" <martin.petersen@oracle.com>,
        Stanley Chu <stanley.chu@mediatek.com>,
        Bean Huo <beanhuo@micron.com>,
        Nitin Rawat <nitirawa@codeaurora.org>,
        Adrian Hunter <adrian.hunter@intel.com>,
        Bart Van Assche <bvanassche@acm.org>,
        Satya Tangirala <satyat@google.com>,
        open list <linux-kernel@vger.kernel.org>,
        Adrian Hunter <adrian.hunter@intel.com>, rjw@rjwysocki.net
Date:   Fri, 08 Jan 2021 12:29:50 +0100
In-Reply-To: <f814b71d1d4ea87a72df4851a8190807@codeaurora.org>
References: <1609595975-12219-1-git-send-email-cang@codeaurora.org>
         <1609595975-12219-3-git-send-email-cang@codeaurora.org>
         <80a15afab8024d0b61d312b57585c9322ac91958.camel@gmail.com>
         <7d49c1dfc3f648c484076f3c3a7f4e1e@codeaurora.org>
         <1514403adf486ac8069253c09f45b021bad32e00.camel@gmail.com>
         <f814b71d1d4ea87a72df4851a8190807@codeaurora.org>
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Wed, 2021-01-06 at 09:20 +0800, Can Guo wrote:
> Hi Bean,
> 
> On 2021-01-06 02:38, Bean Huo wrote:
> > On Tue, 2021-01-05 at 09:07 +0800, Can Guo wrote:
> > > On 2021-01-05 04:05, Bean Huo wrote:
> > > > On Sat, 2021-01-02 at 05:59 -0800, Can Guo wrote:
> > > > > + * @shutting_down: flag to check if shutdown has been
> > > > > invoked
> > > > 
> > > > I am not much sure if this flag is need, since once PM going in
> > > > shutdown path, what will be returnded by pm_runtime_get_sync()?
> > > > 
> > > > If pm_runtime_get_sync() will fail, just check its return.
> > > > 
> > > 
> > > That depends. During/after shutdown, for UFS's case only,
> > > pm_runtime_get_sync(hba->dev) will most likely return 0,
> > > because it is already RUNTIME_ACTIVE, pm_runtime_get_sync()
> > > will directly return 0... meaning you cannot count on it.
> > > 
> > > Check Stanley's change -
> > > https://lore.kernel.org/patchwork/patch/1341389/
> > > 
> > > Can Guo.
> > 
> > Can,
> > 
> > Thanks for pointing out that.
> > 
> > Based on my understanding, that patch is redundent. maybe I
> > misundestood Linux shutdown sequence.
> 
> Sorry, do you mean Stanley's change is redundant?

yes.

> 
> > 
> > I checked the shutdown flow:
> > 
> > 1. Set the "system_state" variable
> > 2. Disable usermod to ensure that no user from userspace can start
> > a
> > request
> 
> I hope it is like what you interpreted, but step #2 only stops
> UMH(#265)
> but not all user space activities. Whereas, UMH is for kernel space 
> calling
> user space.


Can,

I did further study and homework on the Linux shutdown in the last few
days. Yes, you are right, usermodehelper_disable() is to prevent
executing the process from the kernel space.

But I didn't reproduce this "maybe" race issue while shutdown. no
matter how I torment my system, once Linux shutdown/halt/reboot starts,
nobody can access the sysfs node. I create 10 processes in the user
space and constantly access UFS sysfs node, also, fio is running in the
background for the normal data read/write. there is a shutdown thread
that will randomly trigger shutdown/halt/reboot. but no race issue
appears.

I don't know if this is a hypothetical issue(the race between shutdown
flow and sysfs node access), it may not really exist in the Linux
envriroment. everytime, the shutdonw flow will be:

e10_sync_handler()->e10_svc()->do_e10_svc()->__do_sys_reboot()-
>kernel_poweroff/kernel_halt()->device_shutdown()->platform_shutdown()-
>ufshcd_platform_shutdown()->ufshcd_shutdown().

I think before going into the kernel shutdown, the userspace cannot
issue new requests anymore. otherwise, this would be a big issue.

pm_runtime_get_sync() will return 0 or failure while shutdown? the
answer is not important now, maybe as you said, it is always 0. But in
my testing, it didn't get there the system has been shutdown. Which
means once shutdonw starts, sysfs node access path cannot reach
pm_runtime_get_sync(). (note, I don't know if sysfs node access thread
has been disabled or not)


Responsibly say, I didn't reproduce this issue on my system (ubuntu),
maybe you are using Android. I am not an expert on this topic, if you
have the best idea on how to reproduce this issue. please please let me
try. appreciate it!!!!!


Thanks,
Bean


> 
> 264     system_state = state;
> 265     usermodehelper_disable();
> 266     device_shutdown();
> 
> Thanks,
> Can Guo.