Re: [PATCH 2/2] scsi: ufs: Protect PM ops and err_handler from user access through sysfs

[Stanley Chu <stanley.chu@mediatek.com>]

Hi Bean,

On Fri, 2021-01-08 at 12:29 +0100, Bean Huo wrote:
> 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!!!!!
> 

One of the racing in my platforms happens due to I/O access triggered
from kernel space, not user space.

Thanks,
Stanley Chu

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