Hello Shane, > -----Original Message----- > From: Seymour, Shane M [mailto:shane.seymour@xxxxxxx] > Sent: Thursday, January 7, 2016 9:58 PM > To: Raghava Aditya Renukunta; JBottomley@xxxxxxxx; linux- > scsi@xxxxxxxxxxxxxxx > Cc: Mahesh Rajashekhara; Murthy Bhat; Gana Sridaran; aacraid@pmc- > sierra.com; Scott Benesh; jthumshirn@xxxxxxx; thenzl@xxxxxxxxxx; David > Carroll > Subject: RE: [PATCH V2 7/9] aacraid: Fix AIF triggered IOP_RESET > > Is there still a race in the code so the same issue can happen even with this > patch? > > When __aac_shutdown is called it clears out the events and stops the kernel > thread and > then it calls aac_send_shutdown which sets adapter_shutdown. The ioctl > path checks > adapter_shutdown then allows the ioctl to proceed so is there still a window > where someone > can get past the checks and restart the kernel thread? To me it looks likely it's > a very small > window but it still appears to be there because you don't prevent ioctl calls > from continuing > until after you've stopped the kernel thread. It seems as though > adapter_shutdown needs to > get set at the very start of __aac_shutdown as well to prevent any more > requests from > being queued when __aac_shutdown gets called. I agree aac_send_shutdown needs to be called first thing in __aac_shutdown, to prevent ioctls from coming through. > Also since one CPU may be setting the value of adapter_shutdown when > another one is > reading it and you don't use any kind of lock to control access to the value are > you going to > have SMP coherency issues with the value of adapter_shutdown? That is you > read it as 0 > because the CPU that changed it to 1 has it in a register and hasn't stored it > yet. For example > in the ioctl checking case: > > static long aac_cfg_ioctl(struct file *file, > unsigned int cmd, unsigned long arg) > { > int ret; > struct aac_dev *aac; > aac = (struct aac_dev *)file->private_data; > if (!capable(CAP_SYS_RAWIO) || aac->adapter_shutdown) > return -EPERM; > mutex_lock(&aac_mutex); > ret = aac_do_ioctl(file->private_data, cmd, (void __user *)arg); > mutex_unlock(&aac_mutex); > > return ret; > } > > It could be loaded speculatively and you don't know for sure that it's value is > correct at > the time you do the test. Yes, that is true, I think making aac->adapter_shutdown as atomic variable will Prevent SMP coherency issues. I will make the necessary changes and resubmit this series. Thank you very much for your Feedback Shane. Regards, Raghava Aditya > Thanks > Shane -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
