by Stanley Chu

[permalink] [raw]

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

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.
>

2021-01-09 04:47:21

On Sat, 2021-01-02 at 05:59 -0800, Can Guo wrote:
> User layer may access sysfs nodes when system PM ops or error handling
> is running, which can cause various problems. Rename eh_sem to host_sem
> and use it to protect PM ops and error handling from user layer intervene.
>
> Signed-off-by: Can Guo <[email protected]>
>

Looks good to me.

Feel free to add
Reviewed-by: Stanley Chu <[email protected]>

2021-01-12 13:03:19

by Bean Huo

[permalink] [raw]

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

On Tue, 2021-01-12 at 08:45 +0800, Can Guo wrote:
> > > > > to
> > > > > talk to UFS, OCP can happen on VCCQ/VCCQ2. This leads to
> > > > > system
> > > > > CRASH
> > > > > too.
> > > >
> > > > the same as above, share the crash log.
> > > >
> > >
> > > If you have hand-on experiences on NoC and/or OCP issues, you
> > > won't
> > > ask
> > > for the crash log. The tricky parts about critial NoC and OCP
> > > issues
> > > is
> >
> > OK, interesting. would you tell me which register access node in
> > ufs-
> > sysfs.c can trigger this crash? let me verify your statement.
> >
> >
>
> I believe I have explained enough to prove we need this change.
>
> If you are really interested in NoC and OCP, feel free to ping me
> on teams, I will show you how to trigger one and what is it like
> on my setup.
>
> Can Guo.

Ok, I no meant to stop or slow down your patch merging,
meant to avoid redundant instruction cycles.

Avri and Stanley have been convinced. let it go at that and merge.

Thanks,
Bean