Messages in this thread | | | Date | Sat, 09 Jan 2021 12:51:45 +0800 | From | Can Guo <> | Subject | Re: [PATCH 2/2] scsi: ufs: Protect PM ops and err_handler from user access through sysfs |
| |
On 2021-01-09 12:45, Can Guo wrote: > On 2021-01-08 19:29, 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. >> > > No, it is definitely needed. As Stanley replied you in another > thread, it is not protecting I/Os from user layer, but from > other subsystems during shutdown. > >>> >>> > >>> > 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!!!!! >> > > When you do a reboot/shutdown/poweroff, how your system behaves highly > depends on how the reboot cmd is implemented in C code under /sbin/. > > On Ubuntu, reboot looks like: > $ reboot --help > reboot [OPTIONS...] [ARG] > > Reboot the system. > > --help Show this help > --halt Halt the machine > -p --poweroff Switch off the machine > --reboot Reboot the machine > -f --force Force immediate halt/power-off/reboot > -w --wtmp-only Don't halt/power-off/reboot, just write wtmp record > -d --no-wtmp Don't write wtmp record > --no-wall Don't send wall message before halt/power-off/reboot > > > On a pure Linux with a initrd RAM FS built from busybox, reboot looks > like: > # reboot --help > BusyBox v1.30.1 (2019-05-24 12:53:36 IST) multi-call binary. > > Usage: reboot [-d DELAY] [-n] [-f] > > Reboot the system > > -d SEC Delay interval > -n Do not sync > -f Force (don't go through init) > > > For example, when you work on a pure Linux with a filesystem built from > busybox, when you hit reboot cmd, halt_main() will be called. And based > on the reboot options passed to reboot cmd, halt_main() behaves > differently. > > A plain reboot cmd does things like sync filesystem, send SIGKILL to > all > processes (except for init), remount all filesytem as read-only and so > on > before invoking linux kernel reboot syscall. In this case, we are safe. > > However, if you do a "reboot -f", halt_main() directly invokes > reboot(). > And with "reboot -f", I can easily reproduce the race condition we are > talking about here - it is not based on imagination. > > Find the patch I used for replication in the attachment, fix conflicts > if any. After boot up, the cmd lines I used are > > # while true; do cat /sys/devices/platform/soc@0/*ufshc*/rpm_lvl; done > & > # reboot -f > > Can Guo.
Oops... forgot the logs:
# # while true; do cat /sys/devices/platform/soc@0/*ufshc*/rpm_lvl; done & 3 3 3 3 .... # reboot -f 3 3 3 .... [ 17.959206] sd 0:0:0:5: [sdf] Synchronizing SCSI cache 3 [ 17.964833] sd 0:0:0:4: [sde] Synchronizing SCSI cache [ 17.970224] sd 0:0:0:3: [sdd] Synchronizing SCSI cache [ 17.975574] sd 0:0:0:2: [sdc] Synchronizing SCSI cache 3 [ 17.981034] sd 0:0:0:1: [sdb] Synchronizing SCSI cache [ 17.986493] sd 0:0:0:0: [sda] Synchronizing SCSI cache 3 [ 17.991870] [DEBUG]ufshcd_shutdown: UFS SHUTDOWN START [ 17.998902] ------------[ cut here ]------------ [ 18.003648] kernel BUG at drivers/scsi/ufs/ufs-sysfs.c:62! [ 18.009286] Internal error: Oops - BUG: 0 [#1] PREEMPT SMP [ 18.034249] pstate: 40c00005 (nZcv daif +PAN +UAO) [ 18.039185] pc : rpm_lvl_show+0x38/0x40 [ 18.043137] lr : dev_attr_show+0x1c/0x58 [ 18.132552] Call trace: [ 18.135076] rpm_lvl_show+0x38/0x40 [ 18.138672] sysfs_kf_seq_show+0xa8/0x140 [ 18.142802] kernfs_seq_show+0x28/0x30 [ 18.146665] seq_read+0x1d8/0x4b0 [ 18.150072] kernfs_fop_read+0x12c/0x1f0 [ 18.154109] do_iter_read+0x184/0x1c0 [ 18.157882] vfs_readv+0x68/0xb0 ....
> >> >> Thanks, >> Bean >> >> >>> >>> 264 system_state = state; >>> 265 usermodehelper_disable(); >>> 266 device_shutdown(); >>> >>> Thanks, >>> Can Guo.
| |