Re: [PATCH 01/10] mtip32xx: Abort secure erase when drive is mounted

2016-02-23 Thread Asai Thambi SP 

On 2/23/2016 6:14 PM, Jens Axboe wrote:
> On 02/23/2016 07:08 PM, Asai Thambi SP wrote:
>> On 2/23/2016 8:11 AM, Jens Axboe wrote:
>>> On Mon, Feb 22 2016, Asai Thambi SP wrote:

 To avoid erasing a device with a mounted filesystem, try to get exclusive
 access to the blkdev object corresponding to the device.
>>>
>>> I don't think this needs to be in the kernel, why not just check from
>>> the official format tool if the device is mounted or not?
>>>
>>
>> The official format tool checks if the device has a mounted filesystem 
>> before starting an erase operation. But with the driver being in kernel, 
>> some customers use hdparm to manage the device. This patch prevents possible 
>> accidental erase through open source tools.
> 
> We generally don't put that kind of policy in the kernel. I can firmware 
> update a drive that is mounted, if I want to shoot myself in the foot, if I 
> want to. The answer is, don't do it...

Agreed. I will drop this patch and resend the rest.

Re: [PATCH 01/10] mtip32xx: Abort secure erase when drive is mounted

2016-02-23 Thread Jens Axboe 

On 02/23/2016 07:08 PM, Asai Thambi SP wrote:

On 2/23/2016 8:11 AM, Jens Axboe wrote:

On Mon, Feb 22 2016, Asai Thambi SP wrote:


To avoid erasing a device with a mounted filesystem, try to get exclusive
access to the blkdev object corresponding to the device.


I don't think this needs to be in the kernel, why not just check from
the official format tool if the device is mounted or not?



The official format tool checks if the device has a mounted filesystem before 
starting an erase operation. But with the driver being in kernel, some 
customers use hdparm to manage the device. This patch prevents possible 
accidental erase through open source tools.


We generally don't put that kind of policy in the kernel. I can firmware 
update a drive that is mounted, if I want to shoot myself in the foot, 
if I want to. The answer is, don't do it...



--
Jens Axboe

Re: [PATCH 01/10] mtip32xx: Abort secure erase when drive is mounted

2016-02-23 Thread Asai Thambi SP 
On 2/23/2016 8:11 AM, Jens Axboe wrote:
> On Mon, Feb 22 2016, Asai Thambi SP wrote:
>>
>> To avoid erasing a device with a mounted filesystem, try to get exclusive
>> access to the blkdev object corresponding to the device.
> 
> I don't think this needs to be in the kernel, why not just check from
> the official format tool if the device is mounted or not?
> 

The official format tool checks if the device has a mounted filesystem before 
starting an erase operation. But with the driver being in kernel, some 
customers use hdparm to manage the device. This patch prevents possible 
accidental erase through open source tools.

Re: [PATCH 01/10] mtip32xx: Abort secure erase when drive is mounted

2016-02-23 Thread Jens Axboe 
On Mon, Feb 22 2016, Asai Thambi SP wrote:
> 
> To avoid erasing a device with a mounted filesystem, try to get exclusive
> access to the blkdev object corresponding to the device.

I don't think this needs to be in the kernel, why not just check from
the official format tool if the device is mounted or not?

-- 
Jens Axboe

[PATCH 01/10] mtip32xx: Abort secure erase when drive is mounted

2016-02-22 Thread Asai Thambi SP 

To avoid erasing a device with a mounted filesystem, try to get exclusive
access to the blkdev object corresponding to the device.

Signed-off-by: Selvan Mani 
Signed-off-by: Rajesh Kumar Sambandam 
Signed-off-by: Asai Thambi S P 
---
 drivers/block/mtip32xx/mtip32xx.c |  113 ++---
 drivers/block/mtip32xx/mtip32xx.h |2 +
 2 files changed, 94 insertions(+), 21 deletions(-)

diff --git a/drivers/block/mtip32xx/mtip32xx.c 
b/drivers/block/mtip32xx/mtip32xx.c
index 9b180db..37690ab 100644
--- a/drivers/block/mtip32xx/mtip32xx.c
+++ b/drivers/block/mtip32xx/mtip32xx.c
@@ -982,6 +982,28 @@ static void mtip_issue_non_ncq_command(struct mtip_port 
*port, int tag)
 port->cmd_issue[MTIP_TAG_INDEX(tag)]);
 }
 
+static inline int mtip_bdev_claim(struct driver_data *dd)
+{
+if (dd->bdev_claimed == false) {
+igrab(dd->bdev->bd_inode);
+if (blkdev_get(dd->bdev, FMODE_EXCL, dd->bdev) < 0) {
+dev_warn(&dd->pdev->dev, "Drive erase aborted due to non-zero 
refcount (%d)\n",
+dd->bdev->bd_holders);
+return -ERESTARTSYS;
+}
+dd->bdev_claimed = true;
+}
+return 0;
+}
+
+static inline void mtip_bdev_unclaim(struct driver_data *dd)
+{
+if (dd->bdev_claimed) {
+blkdev_put(dd->bdev, FMODE_EXCL);
+dd->bdev_claimed = false;
+}
+}
+
 static bool mtip_pause_ncq(struct mtip_port *port,
 struct host_to_dev_fis *fis)
 {
@@ -991,10 +1013,21 @@ static bool mtip_pause_ncq(struct mtip_port *port,
 reply = port->rxfis + RX_FIS_D2H_REG;
 task_file_data = readl(port->mmio+PORT_TFDATA);
 
-if ((task_file_data & 1))
+if ((task_file_data & 1)) {
+if ((fis->command == ATA_CMD_SEC_SET_PASS) ||
+(fis->command == ATA_CMD_SEC_ERASE_PREP) ||
+(fis->command == ATA_CMD_SEC_ERASE_UNIT) ||
+((fis->command == 0xFC) &&
+(fis->features == 0x27 || fis->features == 0x72 ||
+ fis->features == 0x62 || fis->features == 0x26 ||
+ fis->features == 0x12)))
+mtip_bdev_unclaim(port->dd);
 return false;
+}
 
-if (fis->command == ATA_CMD_SEC_ERASE_PREP) {
+if (fis->command == ATA_CMD_SEC_SET_PASS) {
+mtip_bdev_unclaim(port->dd);
+} else if (fis->command == ATA_CMD_SEC_ERASE_PREP) {
 port->ic_pause_timer = jiffies;
 return true;
 } else if ((fis->command == ATA_CMD_DOWNLOAD_MICRO) &&
@@ -1005,8 +1038,10 @@ static bool mtip_pause_ncq(struct mtip_port *port,
 } else if ((fis->command == ATA_CMD_SEC_ERASE_UNIT) ||
 ((fis->command == 0xFC) &&
 (fis->features == 0x27 || fis->features == 0x72 ||
- fis->features == 0x62 || fis->features == 0x26))) {
+ fis->features == 0x62 || fis->features == 0x26 ||
+ fis->features == 0x12))) {
 clear_bit(MTIP_DDF_SEC_LOCK_BIT, &port->dd->dd_flag);
+mtip_bdev_unclaim(port->dd);
 /* Com reset after secure erase or lowlevel format */
 mtip_restart_port(port);
 clear_bit(MTIP_PF_SE_ACTIVE_BIT, &port->flags);
@@ -1110,8 +1145,23 @@ static int mtip_exec_internal_command(struct mtip_port 
*port,
 
 set_bit(MTIP_PF_IC_ACTIVE_BIT, &port->flags);
 
+/* Check for secure erase while fs mounted */
+if ((fis->command == ATA_CMD_SEC_SET_PASS) ||
+(fis->command == ATA_CMD_SEC_ERASE_PREP) ||
+(fis->command == ATA_CMD_SEC_ERASE_UNIT) ||
+((fis->command == 0xFC) &&
+(fis->features == 0x27 || fis->features == 0x72 ||
+ fis->features == 0x62 || fis->features == 0x26 ||
+ fis->features == 0x12))) {
+rv = mtip_bdev_claim(dd);
+if (rv)
+goto exec_ic_exit;
+}
+
 if (fis->command == ATA_CMD_SEC_ERASE_PREP)
 set_bit(MTIP_PF_SE_ACTIVE_BIT, &port->flags);
+else if (fis->command == ATA_CMD_SEC_ERASE_UNIT)
+dd->port->ic_pause_timer = 0;
 
 clear_bit(MTIP_PF_DM_ACTIVE_BIT, &port->flags);
 
@@ -1122,6 +1172,8 @@ static int mtip_exec_internal_command(struct mtip_port 
*port,
 MTIP_QUIESCE_IO_TIMEOUT_MS) < 0) {
 dev_warn(&dd->pdev->dev,
 "Failed to quiesce IO\n");
+
+mtip_bdev_unclaim(dd);
 mtip_put_int_command(dd, int_cmd);
 clear_bit(MTIP_PF_IC_ACTIVE_BIT, &port->flags);
 wake_up_interruptible(&port->svc_wait);
@@ -1197,6 +1249,8 @@ static int mtip_exec_internal_command(struct mtip_port 
*port,
 mtip_device_reset(dd); /* recover from timeout issue */
 rv = -EAGAIN;
 goto exec_ic_exit;
+} else {
+rv = 0;
 }
 } else {
 u32 hba_stat, port_stat;
@@ -1247,6 +1301,10 @@ static int mtip_exec_internal_command(struct mtip_port 
*port,
 }
 }
 exec_ic_exit:
+if (rv < 0) {
+clear_bit(MTIP_PF_SE_ACTIVE_BIT, &port