| Message ID | 1452151344-417-8-git-send-email-RaghavaAditya.Renukunta@pmcs.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
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.
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.
Thanks
Shane
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Shane, > -----Original Message----- > From: Seymour, Shane M [mailto:shane.seymour@hpe.com] > Sent: Thursday, January 7, 2016 9:58 PM > To: Raghava Aditya Renukunta; JBottomley@odin.com; linux- > scsi@vger.kernel.org > Cc: Mahesh Rajashekhara; Murthy Bhat; Gana Sridaran; aacraid@pmc- > sierra.com; Scott Benesh; jthumshirn@suse.de; thenzl@redhat.com; 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/scsi/aacraid/comminit.c b/drivers/scsi/aacraid/comminit.c index 0e954e3..fe08854 100644 --- a/drivers/scsi/aacraid/comminit.c +++ b/drivers/scsi/aacraid/comminit.c @@ -212,8 +212,9 @@ int aac_send_shutdown(struct aac_dev * dev) return -ENOMEM; aac_fib_init(fibctx); - cmd = (struct aac_close *) fib_data(fibctx); + dev->adapter_shutdown = 1; + cmd = (struct aac_close *) fib_data(fibctx); cmd->command = cpu_to_le32(VM_CloseAll); cmd->cid = cpu_to_le32(0xfffffffe); @@ -229,7 +230,6 @@ int aac_send_shutdown(struct aac_dev * dev) /* FIB should be freed only after getting the response from the F/W */ if (status != -ERESTARTSYS) aac_fib_free(fibctx); - dev->adapter_shutdown = 1; if ((dev->pdev->device == PMC_DEVICE_S7 || dev->pdev->device == PMC_DEVICE_S8 || dev->pdev->device == PMC_DEVICE_S9) &&