[PATCH] pstore: Fail to unlink if a driver has not defined pstore_erase

[PATCH] pstore: Fail to unlink if a driver has not defined pstore_erase

[Aruna Balakrishnaiah]

pstore_erase is used to erase the record from the persistent store.
So if a driver has not defined pstore_erase callback return
-EINVAL instead of unlinking a file as deleting the file without
erasing its record in persistent store will give a wrong impression
to customers.

Signed-off-by: Aruna Balakrishnaiah <aruna@linux.vnet.ibm.com>
---
 fs/pstore/inode.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
index e4bcb2c..fa6339a 100644
--- a/fs/pstore/inode.c
+++ b/fs/pstore/inode.c
@@ -178,6 +178,8 @@ static int pstore_unlink(struct inode *dir, struct dentry *dentry)
 	if (p->psi->erase)
 		p->psi->erase(p->type, p->id, p->count,
 			      dentry->d_inode->i_ctime, p->psi);
+	else
+		return -EINVAL;
 
 	return simple_unlink(dir, dentry);
 }

Re: [PATCH] pstore: Fail to unlink if a driver has not defined pstore_erase

[Kees Cook]

On Mon, Jun 24, 2013 at 12:48 AM, Aruna Balakrishnaiah
<aruna@linux.vnet.ibm.com> wrote:
> pstore_erase is used to erase the record from the persistent store.
> So if a driver has not defined pstore_erase callback return
> -EINVAL instead of unlinking a file as deleting the file without
> erasing its record in persistent store will give a wrong impression
> to customers.

This is probably true -- I originally liked the idea of being able to
clean up the entries, regardless of their storage state, but you're
probably right. They shouldn't be deleted unless they can _actually_
be deleted.

So, I support this change, but I think the return needs to be
different. EINVAL isn't listed, for example, in unlink(2)'s man-page.
Perhaps EROFS, EACCESS, or EPERM?

-Kees

>
> Signed-off-by: Aruna Balakrishnaiah <aruna@linux.vnet.ibm.com>
> ---
>  fs/pstore/inode.c |    2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
> index e4bcb2c..fa6339a 100644
> --- a/fs/pstore/inode.c
> +++ b/fs/pstore/inode.c
> @@ -178,6 +178,8 @@ static int pstore_unlink(struct inode *dir, struct dentry *dentry)
>         if (p->psi->erase)
>                 p->psi->erase(p->type, p->id, p->count,
>                               dentry->d_inode->i_ctime, p->psi);
> +       else
> +               return -EINVAL;
>
>         return simple_unlink(dir, dentry);
>  }
>



--
Kees Cook
Chrome OS Security

Re: [PATCH] pstore: Fail to unlink if a driver has not defined pstore_erase

[Aruna Balakrishnaiah]

Hi Keek,

On Monday 24 June 2013 10:33 PM, Kees Cook wrote:
> On Mon, Jun 24, 2013 at 12:48 AM, Aruna Balakrishnaiah
> <aruna@linux.vnet.ibm.com> wrote:
>> pstore_erase is used to erase the record from the persistent store.
>> So if a driver has not defined pstore_erase callback return
>> -EINVAL instead of unlinking a file as deleting the file without
>> erasing its record in persistent store will give a wrong impression
>> to customers.
> This is probably true -- I originally liked the idea of being able to
> clean up the entries, regardless of their storage state, but you're
> probably right. They shouldn't be deleted unless they can _actually_
> be deleted.
>
> So, I support this change, but I think the return needs to be
> different. EINVAL isn't listed, for example, in unlink(2)'s man-page.
> Perhaps EROFS, EACCESS, or EPERM?

The filesystem (pstore) has privileges to unlink the file but only if the
callback function is defined. Since the filesystem has privileges I didn't
consider these error codes (EROFS, EACCESS or EPERM).

In the case where callback function is not defined unlinking the file would
be an invalid operation and hence EINVAL.

Since unlink(2) man page does not have EINVAL listed, I feel going with
EPERM will make more sense.


>
> -Kees
>
>> Signed-off-by: Aruna Balakrishnaiah <aruna@linux.vnet.ibm.com>
>> ---
>>   fs/pstore/inode.c |    2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
>> index e4bcb2c..fa6339a 100644
>> --- a/fs/pstore/inode.c
>> +++ b/fs/pstore/inode.c
>> @@ -178,6 +178,8 @@ static int pstore_unlink(struct inode *dir, struct dentry *dentry)
>>          if (p->psi->erase)
>>                  p->psi->erase(p->type, p->id, p->count,
>>                                dentry->d_inode->i_ctime, p->psi);
>> +       else
>> +               return -EINVAL;
>>
>>          return simple_unlink(dir, dentry);
>>   }
>>
>
>
> --
> Kees Cook
> Chrome OS Security
>