[PATCH 1/3] f2fs: use wrapped IS_SWAPFILE()

2019-08-15 Thread Chao Yu
Just cleanup, no logic change.

Signed-off-by: Chao Yu 
---
 fs/f2fs/f2fs.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 73e007dca3bb..d2b718e33f88 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -3706,7 +3706,7 @@ static inline bool f2fs_force_buffered_io(struct inode 
*inode,
block_unaligned_IO(inode, iocb, iter))
return true;
if (is_sbi_flag_set(F2FS_I_SB(inode), SBI_CP_DISABLED) &&
-   !(inode->i_flags & S_SWAPFILE))
+   !IS_SWAPFILE(inode))
return true;
 
return false;
-- 
2.18.0.rc1



[PATCH 3/3] f2fs: fix to use more generic EOPNOTSUPP

2019-08-15 Thread Chao Yu
EOPNOTSUPP is widely used as error number indicating operation is
not supported in syscall, and ENOTSUPP was defined and only used
for NFSv3 protocol, so use EOPNOTSUPP instead.

Fixes: 0a2aa8fbb969 ("f2fs: refactor __exchange_data_block for speed up")
Signed-off-by: Chao Yu 
---
 fs/f2fs/file.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index b3937a6497d9..64469555e774 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -1034,7 +1034,7 @@ static int __read_out_blkaddrs(struct inode *inode, 
block_t *blkaddr,
 
if (test_opt(sbi, LFS)) {
f2fs_put_dnode();
-   return -ENOTSUPP;
+   return -EOPNOTSUPP;
}
 
/* do not invalidate this block address */
-- 
2.18.0.rc1



[PATCH 2/3] f2fs: use wrapped f2fs_cp_error()

2019-08-15 Thread Chao Yu
Just cleanup, no logic change.

Signed-off-by: Chao Yu 
---
 fs/f2fs/inode.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
index 5d78f2db7a67..88af85e0db62 100644
--- a/fs/f2fs/inode.c
+++ b/fs/f2fs/inode.c
@@ -706,7 +706,7 @@ void f2fs_evict_inode(struct inode *inode)
stat_dec_inline_dir(inode);
stat_dec_inline_inode(inode);
 
-   if (likely(!is_set_ckpt_flags(sbi, CP_ERROR_FLAG) &&
+   if (likely(!f2fs_cp_error(sbi) &&
!is_sbi_flag_set(sbi, SBI_CP_DISABLED)))
f2fs_bug_on(sbi, is_inode_flag_set(inode, FI_DIRTY_INODE));
else
-- 
2.18.0.rc1



Re: [f2fs-dev] [PATCH 2/2] mkfs.f2fs: add VM disk files to hot data types

2019-08-15 Thread Chao Yu
Hi Ju Hyung,

On 2019/8/14 17:22, Ju Hyung Park wrote:
> Hi Chao,
> 
> You're right and Android will never use those types.
> 
> But then again, what's the point of separating the list?

I'm just thinking to use extension list space more efficiently to avoid free
slot exhaustion.

And in android, let f2fs_create() always to compare nonexistent extension name
is unneeded, right?

> I haven't encountered an Android user or an OEM that wants to
> customize this list by passing an argument to mkfs.f2fs.

That sounds sad for this interface's low use frequency, except Google and
Huawei, it looks f2fs needs more stable users first, now vendor like Huawei,
just use default list (had upstreamed .db as hot type as default yet). :(

> 
> If an OEM want to customize this list, directly modifying the code for
> mkfs.f2fs sounds better anyways.

Yes, also we support adding/deleting extensions via sys node.

Thanks,

> 
> Thanks.
> 
> On Wed, Aug 14, 2019 at 10:51 AM Chao Yu  wrote:
>>
>> On 2019/8/13 6:52, Park Ju Hyung wrote:
>>> Similar to .db files, these are randomly updated extremely frequently.
>>
>> It looks android doesn't need this, how about adding them under "#ifndef
>> WITH_ANDROID"?
>>
>> Thanks,
>>
>>>
>>> Signed-off-by: Park Ju Hyung 
>>> ---
>>>  mkfs/f2fs_format.c | 5 +
>>>  1 file changed, 5 insertions(+)
>>>
>>> diff --git a/mkfs/f2fs_format.c b/mkfs/f2fs_format.c
>>> index 37d82c3..1c08e3e 100644
>>> --- a/mkfs/f2fs_format.c
>>> +++ b/mkfs/f2fs_format.c
>>> @@ -94,6 +94,11 @@ const char *media_ext_lists[] = {
>>>
>>>  const char *hot_ext_lists[] = {
>>>   "db",
>>> +
>>> + /* Virtual machines */
>>> + "vmdk", // VMware or VirtualBox
>>> + "vdi", // VirtualBox
>>> + "qcow2", // QEMU
>>>   NULL
>>>  };
>>>
>>>
> .
> 


___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel


[f2fs-dev] [PATCH v4] Add flags option to get xattr method paired to __vfs_getxattr

2019-08-15 Thread Mark Salyzyn via Linux-f2fs-devel
Add a flag option to get xattr method that could have a bit flag of
XATTR_NOSECURITY passed to it.  XATTR_NOSECURITY is generally then
set in the __vfs_getxattr path.

This handles the case of a union filesystem driver that is being
requested by the security layer to report back the xattr data.

For the use case where access is to be blocked by the security layer.

The path then could be security(dentry) ->
__vfs_getxattr(dentry...XATTR_NOSECUIRTY) ->
handler->get(dentry...XATTR_NOSECURITY) ->
__vfs_getxattr(lower_dentry...XATTR_NOSECUIRTY) ->
lower_handler->get(lower_dentry...XATTR_NOSECUIRTY)
which would report back through the chain data and success as
expected, the logging security layer at the top would have the
data to determine the access permissions and report back the target
context that was blocked.

Without the get handler flag, the path on a union filesystem would be
the errant security(dentry) -> __vfs_getxattr(dentry) ->
handler->get(dentry) -> vfs_getxattr(lower_dentry) -> nested ->
security(lower_dentry, log off) -> lower_handler->get(lower_dentry)
which would report back through the chain no data, and -EACCES.

For selinux for both cases, this would translate to a correctly
determined blocked access. In the first case with this change a correct avc
log would be reported, in the second legacy case an incorrect avc log
would be reported against an uninitialized u:object_r:unlabeled:s0
context making the logs cosmetically useless for audit2allow.

This patch series is inert and is the wide-spread addition of the
flags option for xattr functions, and a replacement of _vfs_getxattr
with __vfs_getxattr(...XATTR_NOSECURITY).

Signed-off-by: Mark Salyzyn 
Cc: Stephen Smalley 
Cc: linux-ker...@vger.kernel.org
Cc: kernel-t...@android.com
Cc: linux-security-mod...@vger.kernel.org
Cc: Jan Kara 
Cc: sta...@vger.kernel.org # 4.4, 4.9, 4.14 & 4.19
---
v4:
- ifdef __KERNEL__ around XATTR_NOSECURITY to
  keep it colocated in uapi headers.

v3:
- poor aim on ubifs not ubifs_xattr_get, but static xattr_get

v2:
- Missed a spot: ubifs, erofs and afs.

v1:
- Removed from an overlayfs patch set, and made independent.
  Expect this to be the basis of some security improvements.
---
 drivers/staging/erofs/xattr.c |  3 ++-
 fs/9p/acl.c   |  3 ++-
 fs/9p/xattr.c |  3 ++-
 fs/afs/xattr.c|  8 +++
 fs/btrfs/xattr.c  |  3 ++-
 fs/ceph/xattr.c   |  3 ++-
 fs/cifs/xattr.c   |  2 +-
 fs/ecryptfs/inode.c   |  6 --
 fs/ecryptfs/mmap.c|  2 +-
 fs/ext2/xattr_trusted.c   |  2 +-
 fs/ext2/xattr_user.c  |  2 +-
 fs/ext4/xattr_security.c  |  2 +-
 fs/ext4/xattr_trusted.c   |  2 +-
 fs/ext4/xattr_user.c  |  2 +-
 fs/f2fs/xattr.c   |  4 ++--
 fs/fuse/xattr.c   |  4 ++--
 fs/gfs2/xattr.c   |  3 ++-
 fs/hfs/attr.c |  2 +-
 fs/hfsplus/xattr.c|  3 ++-
 fs/hfsplus/xattr_trusted.c|  3 ++-
 fs/hfsplus/xattr_user.c   |  3 ++-
 fs/jffs2/security.c   |  3 ++-
 fs/jffs2/xattr_trusted.c  |  3 ++-
 fs/jffs2/xattr_user.c |  3 ++-
 fs/jfs/xattr.c|  5 +++--
 fs/kernfs/inode.c |  3 ++-
 fs/nfs/nfs4proc.c |  6 --
 fs/ocfs2/xattr.c  |  9 +---
 fs/orangefs/xattr.c   |  3 ++-
 fs/overlayfs/super.c  |  8 ---
 fs/posix_acl.c|  2 +-
 fs/reiserfs/xattr_security.c  |  3 ++-
 fs/reiserfs/xattr_trusted.c   |  3 ++-
 fs/reiserfs/xattr_user.c  |  3 ++-
 fs/squashfs/xattr.c   |  2 +-
 fs/ubifs/xattr.c  |  3 ++-
 fs/xattr.c| 36 +++
 fs/xfs/xfs_xattr.c|  3 ++-
 include/linux/xattr.h |  9 
 include/uapi/linux/xattr.h|  7 --
 mm/shmem.c|  3 ++-
 net/socket.c  |  3 ++-
 security/commoncap.c  |  6 --
 security/integrity/evm/evm_main.c |  3 ++-
 security/selinux/hooks.c  | 11 ++
 security/smack/smack_lsm.c|  5 +++--
 46 files changed, 126 insertions(+), 84 deletions(-)

diff --git a/drivers/staging/erofs/xattr.c b/drivers/staging/erofs/xattr.c
index df40654b9fbb..69440065432c 100644
--- a/drivers/staging/erofs/xattr.c
+++ b/drivers/staging/erofs/xattr.c
@@ -463,7 +463,8 @@ int erofs_getxattr(struct inode *inode, int index,
 
 static int erofs_xattr_generic_get(const struct xattr_handler *handler,
   struct dentry *unused, struct inode *inode,
-  const char *name, void *buffer, size_t size)
+  const char *name, void *buffer, size_t size,
+  int flags)
 {
struct 

Re: [f2fs-dev] [PATCH v2] Add flags option to get xattr method paired to __vfs_getxattr

2019-08-15 Thread Jan Kara
On Wed 14-08-19 07:54:16, Mark Salyzyn wrote:
> On 8/14/19 4:00 AM, Jan Kara wrote:
> > On Tue 13-08-19 07:55:06, Mark Salyzyn wrote:
> > ...
> > > diff --git a/fs/xattr.c b/fs/xattr.c
> > > index 90dd78f0eb27..71f887518d6f 100644
> > > --- a/fs/xattr.c
> > > +++ b/fs/xattr.c
> > ...
> > >   ssize_t
> > >   __vfs_getxattr(struct dentry *dentry, struct inode *inode, const char 
> > > *name,
> > > -void *value, size_t size)
> > > +void *value, size_t size, int flags)
> > >   {
> > >   const struct xattr_handler *handler;
> > > -
> > > - handler = xattr_resolve_name(inode, );
> > > - if (IS_ERR(handler))
> > > - return PTR_ERR(handler);
> > > - if (!handler->get)
> > > - return -EOPNOTSUPP;
> > > - return handler->get(handler, dentry, inode, name, value, size);
> > > -}
> > > -EXPORT_SYMBOL(__vfs_getxattr);
> > > -
> > > -ssize_t
> > > -vfs_getxattr(struct dentry *dentry, const char *name, void *value, 
> > > size_t size)
> > > -{
> > > - struct inode *inode = dentry->d_inode;
> > >   int error;
> > > + if (flags & XATTR_NOSECURITY)
> > > + goto nolsm;
> > Hum, is it OK for XATTR_NOSECURITY to skip even the xattr_permission()
> > check? I understand that for reads of security xattrs it actually does not
> > matter in practice but conceptually that seems wrong to me as
> > XATTR_NOSECURITY is supposed to skip just security-module checks to avoid
> > recursion AFAIU.
> 
> Good catch I think.
> 
> I was attempting to make this change purely inert, no change in
> functionality, only a change in API. Adding a call to xattr_permission would
> incur a change in overall functionality, as it would introduce into the
> current and original __vfs_getxattr a call to xattr_permission that was not
> there before.
> 
> (I will have to defer the real answer and requirements to the security
> folks)
> 
> AFAIK you are correct, and to make the call would reduce the attack surface,
> trading a very small amount of CPU utilization, for a much larger amount of
> trust.
> 
> Given the long history of this patch set (for overlayfs) and the large
> amount of stakeholders, I would _prefer_ to submit a followup independent
> functionality/security change to _vfs_get_xattr _after_ this makes it in.

You're right. The problem was there before. So ack to changing this later.

> > > diff --git a/include/uapi/linux/xattr.h b/include/uapi/linux/xattr.h
> > > index c1395b5bd432..1216d777d210 100644
> > > --- a/include/uapi/linux/xattr.h
> > > +++ b/include/uapi/linux/xattr.h
> > > @@ -17,8 +17,9 @@
> > >   #if __UAPI_DEF_XATTR
> > >   #define __USE_KERNEL_XATTR_DEFS
> > > -#define XATTR_CREATE 0x1 /* set value, fail if attr already 
> > > exists */
> > > -#define XATTR_REPLACE0x2 /* set value, fail if attr does not 
> > > exist */
> > > +#define XATTR_CREATE  0x1/* set value, fail if attr already 
> > > exists */
> > > +#define XATTR_REPLACE 0x2/* set value, fail if attr does not 
> > > exist */
> > > +#define XATTR_NOSECURITY 0x4 /* get value, do not involve security 
> > > check */
> > >   #endif
> > It seems confusing to export XATTR_NOSECURITY definition to userspace when
> > that is kernel-internal flag. I'd just define it in include/linux/xattr.h
> > somewhere from the top of flags space (like 0x4000).
> > 
> > Otherwise the patch looks OK to me (cannot really comment on the security
> > module aspect of this whole thing though).
> 
> Good point. However, we do need to keep these flags together to reduce
> maintenance risk, I personally abhor two locations for flags bits even if
> one comes from the opposite bit-side; collisions are undetectable at build
> time. Although I have not gone through the entire thought experiment, I am
> expecting that fuse could possibly benefit from this flag (if exposed) since
> it also has a security recursion. That said, fuse is probably the example of
> a gaping wide attack surface if user space had access to it ... your
> xattr_permissions call addition requested above would be realistically, not
> just pedantically, required!

Yeah, flags bits in two places are bad as well. So maybe at least
#ifdef __KERNEL__ bit around the definitiona and a comment that it is
kernel internal flag?

Honza
-- 
Jan Kara 
SUSE Labs, CR


___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel


Re: [f2fs-dev] [PATCH] Add flags option to get xattr method paired to __vfs_getxattr

2019-08-15 Thread James Morris
On Thu, 15 Aug 2019, Mark Salyzyn wrote:

> Good Idea, but using the same argument structure for set and get I would be
> concerned about the loss of compiler protection for the buffer argument;

Agreed, I missed that.

> struct getxattr_args {
>   struct dentry *dentry;
>   struct inode *inode;
>   const char *name;
>   void *buffer;
>   size_t size;
>   int flags;

Does 'get' need flags?

-- 
James Morris




___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel


Re: [f2fs-dev] [PATCH v2] mkfs.f2fs: check zeros in first 16MB for Android

2019-08-15 Thread Jaegeuk Kim
On 08/12, Chao Yu wrote:
> On 2019/8/9 23:12, Jaegeuk Kim wrote:
> > We actually don't need to issue trim on entire disk by checking first
> > blocks having zeros.
> 
> In heap mode, we locate node log header to tail end of device, should we
> consider to check block contain according to heap option?

I wanted to check F2FS metadata mainly.

> 
> BTW, if we changed cp_ver whenever mkfs, why should we still issue trim to
> obsolete old data in node remained in image?

For simplicity. :P

> 
> Thanks,
> 
> > 
> > Signed-off-by: Jaegeuk Kim 
> > ---
> > v2 from v1:
> >  - clean up
> > 
> >  mkfs/f2fs_format_utils.c | 53 ++--
> >  1 file changed, 51 insertions(+), 2 deletions(-)
> > 
> > diff --git a/mkfs/f2fs_format_utils.c b/mkfs/f2fs_format_utils.c
> > index 8bf128c..f2d55ad 100644
> > --- a/mkfs/f2fs_format_utils.c
> > +++ b/mkfs/f2fs_format_utils.c
> > @@ -25,6 +25,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  #ifndef ANDROID_WINDOWS_HOST
> >  #include 
> >  #endif
> > @@ -110,13 +111,61 @@ static int trim_device(int i)
> > return 0;
> >  }
> >  
> > +static bool is_wiped_device(int i)
> > +{
> > +#ifdef WITH_ANDROID
> > +   struct device_info *dev = c.devices + i;
> > +   int fd = dev->fd;
> > +   char *buf, *zero_buf;
> > +   bool wiped = true;
> > +   int nblocks = 4096; /* 16MB size */
> > +   int j;
> > +
> > +   buf = malloc(F2FS_BLKSIZE);
> > +   if (buf == NULL) {
> > +   MSG(1, "\tError: Malloc Failed for buf!!!\n");
> > +   return false;
> > +   }
> > +   zero_buf = calloc(1, F2FS_BLKSIZE);
> > +   if (zero_buf == NULL) {
> > +   MSG(1, "\tError: Calloc Failed for zero buf!!!\n");
> > +   free(buf);
> > +   return false;
> > +   }
> > +
> > +   if (lseek(fd, 0, SEEK_SET) < 0) {
> > +   free(zero_buf);
> > +   free(buf);
> > +   return false;
> > +   }
> > +
> > +   /* check first n blocks */
> > +   for (j = 0; j < nblocks; j++) {
> > +   if (read(fd, buf, F2FS_BLKSIZE) != F2FS_BLKSIZE ||
> > +   memcmp(buf, zero_buf, F2FS_BLKSIZE)) {
> > +   wiped = false;
> > +   break;
> > +   }
> > +   }
> > +   free(zero_buf);
> > +   free(buf);
> > +
> > +   if (wiped)
> > +   MSG(0, "Info: Found all zeros in first %d blocks\n", nblocks);
> > +   return wiped;
> > +#else
> > +   return false;
> > +#endif
> > +}
> > +
> >  int f2fs_trim_devices(void)
> >  {
> > int i;
> >  
> > -   for (i = 0; i < c.ndevs; i++)
> > -   if (trim_device(i))
> > +   for (i = 0; i < c.ndevs; i++) {
> > +   if (!is_wiped_device(i) && trim_device(i))
> > return -1;
> > +   }
> > c.trimmed = 1;
> > return 0;
> >  }
> > 


___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel


Re: [f2fs-dev] [PATCH 1/4] fsck.f2fs: fix to recover out-of-border inline size

2019-08-15 Thread Jaegeuk Kim
On 08/12, Chao Yu wrote:
> It tries to let fsck be noticed wrong inline size, and do the fix.
> 
> Signed-off-by: Chao Yu 
> ---
>  fsck/fsck.c | 8 
>  1 file changed, 8 insertions(+)
> 
> diff --git a/fsck/fsck.c b/fsck/fsck.c
> index d53317c..7eb599d 100644
> --- a/fsck/fsck.c
> +++ b/fsck/fsck.c
> @@ -771,6 +771,8 @@ void fsck_chk_inode_blk(struct f2fs_sb_info *sbi, u32 nid,
>   ofs = get_extra_isize(node_blk);
>  
>   if ((node_blk->i.i_inline & F2FS_INLINE_DATA)) {
> + unsigned int inline_size = MAX_INLINE_DATA(node_blk);
> +
>   if (le32_to_cpu(node_blk->i.i_addr[ofs]) != 0) {
>   /* should fix this bug all the time */
>   FIX_MSG("inline_data has wrong 0'th block = %x",
> @@ -779,6 +781,12 @@ void fsck_chk_inode_blk(struct f2fs_sb_info *sbi, u32 
> nid,
>   node_blk->i.i_blocks = cpu_to_le64(*blk_cnt);
>   need_fix = 1;
>   }
> + if (!i_size || i_size > inline_size) {

i_size=0 should be fine?

> + node_blk->i.i_size = cpu_to_le64(inline_size);
> + FIX_MSG("inline_data has wrong i_size %lu",
> + (unsigned long)i_size);
> + need_fix = 1;
> + }
>   if (!(node_blk->i.i_inline & F2FS_DATA_EXIST)) {
>   char buf[MAX_INLINE_DATA(node_blk)];
>   memset(buf, 0, MAX_INLINE_DATA(node_blk));
> -- 
> 2.18.0.rc1


___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel


Re: [f2fs-dev] [PATCH] Add flags option to get xattr method paired to __vfs_getxattr

2019-08-15 Thread James Morris
On Tue, 13 Aug 2019, Greg Kroah-Hartman wrote:

> On Mon, Aug 12, 2019 at 12:32:49PM -0700, Mark Salyzyn wrote:
> > --- a/include/linux/xattr.h
> > +++ b/include/linux/xattr.h
> > @@ -30,10 +30,10 @@ struct xattr_handler {
> > const char *prefix;
> > int flags;  /* fs private flags */
> > bool (*list)(struct dentry *dentry);
> > -   int (*get)(const struct xattr_handler *, struct dentry *dentry,
> > +   int (*get)(const struct xattr_handler *handler, struct dentry *dentry,
> >struct inode *inode, const char *name, void *buffer,
> > -  size_t size);
> > -   int (*set)(const struct xattr_handler *, struct dentry *dentry,
> > +  size_t size, int flags);
> > +   int (*set)(const struct xattr_handler *handler, struct dentry *dentry,
> >struct inode *inode, const char *name, const void *buffer,
> >size_t size, int flags);
> 
> Wow, 7 arguments.  Isn't there some nice rule of thumb that says once
> you get more then 5, a function becomes impossible to understand?
> 
> Surely this could be a structure passed in here somehow, that way when
> you add the 8th argument in the future, you don't have to change
> everything yet again?  :)
> 
> I don't have anything concrete to offer as a replacement fix for this,
> but to me this just feels really wrong...

How about something like:

struct xattr_gs_args {
struct dentry *dentry;
struct inode *inode;
const char *name;
const void *buffer;
size_t size;
int flags;
};

int (*get)(const struct xattr_handler *handler, struct xattr_gs_args *args);
int (*set)(const struct xattr_handler *handler, struct xattr_gs_args *args);


-- 
James Morris




___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel


Re: [f2fs-dev] [PATCH] Add flags option to get xattr method paired to __vfs_getxattr

2019-08-15 Thread Greg Kroah-Hartman
On Fri, Aug 16, 2019 at 05:20:36AM +1000, James Morris wrote:
> On Tue, 13 Aug 2019, Greg Kroah-Hartman wrote:
> 
> > On Mon, Aug 12, 2019 at 12:32:49PM -0700, Mark Salyzyn wrote:
> > > --- a/include/linux/xattr.h
> > > +++ b/include/linux/xattr.h
> > > @@ -30,10 +30,10 @@ struct xattr_handler {
> > >   const char *prefix;
> > >   int flags;  /* fs private flags */
> > >   bool (*list)(struct dentry *dentry);
> > > - int (*get)(const struct xattr_handler *, struct dentry *dentry,
> > > + int (*get)(const struct xattr_handler *handler, struct dentry *dentry,
> > >  struct inode *inode, const char *name, void *buffer,
> > > -size_t size);
> > > - int (*set)(const struct xattr_handler *, struct dentry *dentry,
> > > +size_t size, int flags);
> > > + int (*set)(const struct xattr_handler *handler, struct dentry *dentry,
> > >  struct inode *inode, const char *name, const void *buffer,
> > >  size_t size, int flags);
> > 
> > Wow, 7 arguments.  Isn't there some nice rule of thumb that says once
> > you get more then 5, a function becomes impossible to understand?
> > 
> > Surely this could be a structure passed in here somehow, that way when
> > you add the 8th argument in the future, you don't have to change
> > everything yet again?  :)
> > 
> > I don't have anything concrete to offer as a replacement fix for this,
> > but to me this just feels really wrong...
> 
> How about something like:
> 
> struct xattr_gs_args {
>   struct dentry *dentry;
>   struct inode *inode;

As he said in a later message, dentry and inode is redundant, only 1 is
needed (dentry I think?)

>   const char *name;
>   const void *buffer;
>   size_t size;
>   int flags;
> };
> 
> int (*get)(const struct xattr_handler *handler, struct xattr_gs_args *args);
> int (*set)(const struct xattr_handler *handler, struct xattr_gs_args *args);

But yes, that would be much much better.

thanks,

greg k-h


___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel


Re: [f2fs-dev] [PATCH 1/4] fsck.f2fs: fix to recover out-of-border inline size

2019-08-15 Thread Jaegeuk Kim
On 08/15, Jaegeuk Kim wrote:
> On 08/12, Chao Yu wrote:
> > It tries to let fsck be noticed wrong inline size, and do the fix.
> > 
> > Signed-off-by: Chao Yu 
> > ---
> >  fsck/fsck.c | 8 
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/fsck/fsck.c b/fsck/fsck.c
> > index d53317c..7eb599d 100644
> > --- a/fsck/fsck.c
> > +++ b/fsck/fsck.c
> > @@ -771,6 +771,8 @@ void fsck_chk_inode_blk(struct f2fs_sb_info *sbi, u32 
> > nid,
> > ofs = get_extra_isize(node_blk);
> >  
> > if ((node_blk->i.i_inline & F2FS_INLINE_DATA)) {
> > +   unsigned int inline_size = MAX_INLINE_DATA(node_blk);
> > +
> > if (le32_to_cpu(node_blk->i.i_addr[ofs]) != 0) {
> > /* should fix this bug all the time */
> > FIX_MSG("inline_data has wrong 0'th block = %x",
> > @@ -779,6 +781,12 @@ void fsck_chk_inode_blk(struct f2fs_sb_info *sbi, u32 
> > nid,
> > node_blk->i.i_blocks = cpu_to_le64(*blk_cnt);
> > need_fix = 1;
> > }
> > +   if (!i_size || i_size > inline_size) {
> 
> i_size=0 should be fine?

Missed v2.

> 
> > +   node_blk->i.i_size = cpu_to_le64(inline_size);
> > +   FIX_MSG("inline_data has wrong i_size %lu",
> > +   (unsigned long)i_size);
> > +   need_fix = 1;
> > +   }
> > if (!(node_blk->i.i_inline & F2FS_DATA_EXIST)) {
> > char buf[MAX_INLINE_DATA(node_blk)];
> > memset(buf, 0, MAX_INLINE_DATA(node_blk));
> > -- 
> > 2.18.0.rc1
> 
> 
> ___
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel


___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel


Re: [f2fs-dev] [PATCH] f2fs_io: add get/setflags

2019-08-15 Thread Jaegeuk Kim
On 08/12, Chao Yu wrote:
> On 2019/8/9 23:23, Jaegeuk Kim wrote:
> > From: Jaegeuk Kim 
> > 
> > Signed-off-by: Jaegeuk Kim 
> > ---
> >  tools/f2fs_io/f2fs_io.c | 91 +
> >  tools/f2fs_io/f2fs_io.h | 14 +++
> >  2 files changed, 105 insertions(+)
> > 
> > diff --git a/tools/f2fs_io/f2fs_io.c b/tools/f2fs_io/f2fs_io.c
> > index 6b43778..b57c458 100644
> > --- a/tools/f2fs_io/f2fs_io.c
> > +++ b/tools/f2fs_io/f2fs_io.c
> > @@ -45,6 +45,95 @@ struct cmd_desc {
> > int cmd_flags;
> >  };
> >  
> > +#define getflags_desc "getflags ioctl"
> > +#define getflags_help  \
> > +"f2fs_io getflags [file]\n\n"  \
> > +"get a flag given the file\n"  \
> > +"flag can show \n" \
> > +"  casefold\n" \
> > +"  compression\n"  \
> > +"  nocompression\n"
> 
> Could we support +/- flags?
> 
> > +
> > +static void do_getflags(int argc, char **argv, const struct cmd_desc *cmd)
> > +{
> > +   long flag;
> > +   int ret, fd;
> > +   int exist = 0;
> > +
> > +   if (argc != 2) {
> > +   fputs("Excess arguments\n\n", stderr);
> > +   fputs(cmd->cmd_help, stderr);
> > +   exit(1);
> > +   }
> > +
> > +   fd = open(argv[1], O_RDONLY);
> > +   if (fd == -1) {
> > +   fputs("Open failed\n\n", stderr);
> > +   fputs(cmd->cmd_help, stderr);
> > +   exit(1);
> > +   }
> > +   ret = ioctl(fd, F2FS_IOC_GETFLAGS, );
> > +   printf("get a flag on %s ret=%d, flags=", argv[1], ret);
> > +   if (flag & FS_CASEFOLD_FL) {
> > +   printf("casefold");
> > +   exist = 1;
> > +   }
> > +   if (flag & FS_COMPR_FL) {
> > +   if (exist)
> > +   printf(",");
> > +   printf("compression");
> > +   exist = 1;
> > +   }
> > +   if (flag & FS_NOCOMP_FL) {
> > +   if (exist)
> > +   printf(",");
> > +   printf("nocompression");
> > +   exist = 1;
> > +   }
> > +   if (!exist)
> > +   printf("none");
> > +   printf("\n");
> > +   exit(0);
> > +}
> > +
> > +#define setflags_desc "setflags ioctl"
> > +#define setflags_help  \
> > +"f2fs_io setflags [flag] [file]\n\n"   \
> > +"set a flag given the file\n"  \
> > +"flag can be\n"\
> > +"  casefold\n" \
> > +"  compression\n"  \
> > +"  nocompression\n"
> > +
> > +static void do_setflags(int argc, char **argv, const struct cmd_desc *cmd)
> > +{
> > +   long flag;
> > +   int ret, fd;
> > +
> > +   if (argc != 3) {
> > +   fputs("Excess arguments\n\n", stderr);
> > +   fputs(cmd->cmd_help, stderr);
> > +   exit(1);
> > +   }
> > +
> > +   fd = open(argv[2], O_RDONLY);
> > +   if (fd == -1) {
> > +   fputs("Open failed\n\n", stderr);
> > +   fputs(cmd->cmd_help, stderr);
> > +   exit(1);
> > +   }
> > +   if (!strcmp(argv[1], "casefold"))
> > +   flag = FS_CASEFOLD_FL;
> > +   else if (!strcmp(argv[1], "compression"))
> > +   flag = FS_COMPR_FL;
> > +   else if (!strcmp(argv[1], "nocompression"))
> > +   flag = FS_NOCOMP_FL;
> > +
> > +   ret = ioctl(fd, F2FS_IOC_SETFLAGS, );
> 
> It will drop other existed flags, should be:

Is it correct?

In f2fs_setflags_common,

fi->i_flags = iflags | (fi->i_flags & ~mask);


> 
> F2FS_IOC_GETFLAGS flag
> flag +/- FS_*_FL
> F2FS_IOC_SETFLAGS flag
> 
> Thanks,
> 
> > +   printf("set a flag: %s ret=%d\n", argv[2], ret);
> > +   exit(0);
> > +}
> > +
> >  #define shutdown_desc "shutdown filesystem"
> >  #define shutdown_help  \
> >  "f2fs_io shutdown [level] [dir]\n\n"   \
> > @@ -488,6 +577,8 @@ static void do_defrag_file(int argc, char **argv, const 
> > struct cmd_desc *cmd)
> >  static void do_help(int argc, char **argv, const struct cmd_desc *cmd);
> >  const struct cmd_desc cmd_list[] = {
> > _CMD(help),
> > +   CMD(getflags),
> > +   CMD(setflags),
> > CMD(shutdown),
> > CMD(pinfile),
> > CMD(fallocate),
> > diff --git a/tools/f2fs_io/f2fs_io.h b/tools/f2fs_io/f2fs_io.h
> > index 95fd5be..5768c1b 100644
> > --- a/tools/f2fs_io/f2fs_io.h
> > +++ b/tools/f2fs_io/f2fs_io.h
> > @@ -41,9 +41,13 @@ typedef u32  __be32;
> >  #ifndef FS_IOC_GETFLAGS
> >  #define FS_IOC_GETFLAGS_IOR('f', 1, long)
> >  #endif
> > +#ifndef FS_IOC_SETFLAGS
> > +#define FS_IOC_SETFLAGS_IOW('f', 2, long)
> > +#endif
> >  
> >  #define F2FS_IOCTL_MAGIC   0xf5
> >  #define F2FS_IOC_GETFLAGS  FS_IOC_GETFLAGS
> > 

Re: [f2fs-dev] [PATCH 2/4] fsck.f2fs: fix to check c.fix_on before repair

2019-08-15 Thread Jaegeuk Kim
On 08/12, Chao Yu wrote:
> We should always set c.bug_on whenever found a bug, then fix them
> if c.fix_on is on, otherwise, some bugs won't be shown unless we
> enable debug log.
> 
> Signed-off-by: Chao Yu 
> ---
>  fsck/fsck.c | 137 +++-
>  1 file changed, 83 insertions(+), 54 deletions(-)
> 
> diff --git a/fsck/fsck.c b/fsck/fsck.c
> index 7eb599d..91ddd49 100644
> --- a/fsck/fsck.c
> +++ b/fsck/fsck.c
> @@ -712,12 +712,13 @@ void fsck_chk_inode_blk(struct f2fs_sb_info *sbi, u32 
> nid,
>   fsck_reada_node_block(sbi, le32_to_cpu(node_blk->i.i_xattr_nid));
>  
>   if (fsck_chk_xattr_blk(sbi, nid,
> - le32_to_cpu(node_blk->i.i_xattr_nid), blk_cnt) &&
> - c.fix_on) {
> - node_blk->i.i_xattr_nid = 0;
> - need_fix = 1;
> - FIX_MSG("Remove xattr block: 0x%x, x_nid = 0x%x",
> - nid, le32_to_cpu(node_blk->i.i_xattr_nid));
> + le32_to_cpu(node_blk->i.i_xattr_nid), blk_cnt)) {
> + if (c.fix_on) {
> + node_blk->i.i_xattr_nid = 0;
> + need_fix = 1;
> + FIX_MSG("Remove xattr block: 0x%x, x_nid = 0x%x",
> + nid, 
> le32_to_cpu(node_blk->i.i_xattr_nid));
> + }

Why do we need this change?

>   }
>  
>   if (ftype == F2FS_FT_CHRDEV || ftype == F2FS_FT_BLKDEV ||
> @@ -730,24 +731,32 @@ void fsck_chk_inode_blk(struct f2fs_sb_info *sbi, u32 
> nid,
>  
>   if (f2fs_has_extra_isize(_blk->i)) {
>   if (c.feature & cpu_to_le32(F2FS_FEATURE_EXTRA_ATTR)) {
> - if (node_blk->i.i_extra_isize >
> - cpu_to_le16(F2FS_TOTAL_EXTRA_ATTR_SIZE)) {
> - FIX_MSG("ino[0x%x] recover i_extra_isize "
> - "from %u to %u",
> - nid,
> - le16_to_cpu(node_blk->i.i_extra_isize),
> - calc_extra_isize());
> - node_blk->i.i_extra_isize =
> - cpu_to_le16(calc_extra_isize());
> - need_fix = 1;
> + unsigned int isize =
> + le16_to_cpu(node_blk->i.i_extra_isize);
> +
> + if (isize > F2FS_TOTAL_EXTRA_ATTR_SIZE) {
> + ASSERT_MSG("[0x%x] wrong i_extra_isize=0x%x",
> + nid, isize);
> + if (c.fix_on) {
> + FIX_MSG("ino[0x%x] recover 
> i_extra_isize "
> + "from %u to %u",
> + nid, isize,
> + calc_extra_isize());
> + node_blk->i.i_extra_isize =
> + cpu_to_le16(calc_extra_isize());
> + need_fix = 1;
> + }
>   }
>   } else {
> - FIX_MSG("ino[0x%x] remove F2FS_EXTRA_ATTR "
> - "flag in i_inline:%u",
> - nid, node_blk->i.i_inline);
> - /* we don't support tuning F2FS_FEATURE_EXTRA_ATTR now 
> */
> - node_blk->i.i_inline &= ~F2FS_EXTRA_ATTR;
> - need_fix = 1;
> + ASSERT_MSG("[0x%x] wrong extra_attr flag", nid);
> + if (c.fix_on) {
> + FIX_MSG("ino[0x%x] remove F2FS_EXTRA_ATTR "
> + "flag in i_inline:%u",
> + nid, node_blk->i.i_inline);
> + /* we don't support tuning 
> F2FS_FEATURE_EXTRA_ATTR now */
> + node_blk->i.i_inline &= ~F2FS_EXTRA_ATTR;
> + need_fix = 1;
> + }
>   }
>  
>   if ((c.feature &
> @@ -758,13 +767,17 @@ void fsck_chk_inode_blk(struct f2fs_sb_info *sbi, u32 
> nid,
>  
>   if (!inline_size ||
>   inline_size > MAX_INLINE_XATTR_SIZE) {
> - FIX_MSG("ino[0x%x] recover inline xattr size "
> - "from %u to %u",
> - nid, inline_size,
> - DEFAULT_INLINE_XATTR_ADDRS);
> - node_blk->i.i_inline_xattr_size =
> - cpu_to_le16(DEFAULT_INLINE_XATTR_ADDRS);
> - need_fix = 1;
> + ASSERT_MSG("[0x%x] wrong inline_xattr_size:%u",
> +

Re: [f2fs-dev] [PATCH v2] mkfs.f2fs: check zeros in first 16MB for Android

2019-08-15 Thread Chao Yu
On 2019/8/16 9:02, Jaegeuk Kim wrote:
> On 08/16, Chao Yu wrote:
>> On 2019/8/16 6:21, Jaegeuk Kim wrote:
>>> On 08/12, Chao Yu wrote:
 On 2019/8/9 23:12, Jaegeuk Kim wrote:
> We actually don't need to issue trim on entire disk by checking first
> blocks having zeros.

 In heap mode, we locate node log header to tail end of device, should we
 consider to check block contain according to heap option?
>>>
>>> I wanted to check F2FS metadata mainly.
>>
>> Oh, I thought you mean main area. :P
>>
>>>

 BTW, if we changed cp_ver whenever mkfs, why should we still issue trim to
 obsolete old data in node remained in image?
>>>
>>> For simplicity. :P
>>
>> I didn't get why we can assume all metadata are zeroed if first 16MB are all 
>> zero...
>>
>> BTW, if first 16MB are non-zero, why not just trim F2FS metadata rather than
>> whole area?
> 
> Trim the entire space so that we can skip discard in runtime by the flag, 
> right?

You're right, thanks helping recall. ;)

Thanks,

> 
>>
>> Thanks,
>>
>>>

 Thanks,

>
> Signed-off-by: Jaegeuk Kim 
> ---
> v2 from v1:
>  - clean up
>
>  mkfs/f2fs_format_utils.c | 53 ++--
>  1 file changed, 51 insertions(+), 2 deletions(-)
>
> diff --git a/mkfs/f2fs_format_utils.c b/mkfs/f2fs_format_utils.c
> index 8bf128c..f2d55ad 100644
> --- a/mkfs/f2fs_format_utils.c
> +++ b/mkfs/f2fs_format_utils.c
> @@ -25,6 +25,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #ifndef ANDROID_WINDOWS_HOST
>  #include 
>  #endif
> @@ -110,13 +111,61 @@ static int trim_device(int i)
>   return 0;
>  }
>  
> +static bool is_wiped_device(int i)
> +{
> +#ifdef WITH_ANDROID
> + struct device_info *dev = c.devices + i;
> + int fd = dev->fd;
> + char *buf, *zero_buf;
> + bool wiped = true;
> + int nblocks = 4096; /* 16MB size */
> + int j;
> +
> + buf = malloc(F2FS_BLKSIZE);
> + if (buf == NULL) {
> + MSG(1, "\tError: Malloc Failed for buf!!!\n");
> + return false;
> + }
> + zero_buf = calloc(1, F2FS_BLKSIZE);
> + if (zero_buf == NULL) {
> + MSG(1, "\tError: Calloc Failed for zero buf!!!\n");
> + free(buf);
> + return false;
> + }
> +
> + if (lseek(fd, 0, SEEK_SET) < 0) {
> + free(zero_buf);
> + free(buf);
> + return false;
> + }
> +
> + /* check first n blocks */
> + for (j = 0; j < nblocks; j++) {
> + if (read(fd, buf, F2FS_BLKSIZE) != F2FS_BLKSIZE ||
> + memcmp(buf, zero_buf, F2FS_BLKSIZE)) {
> + wiped = false;
> + break;
> + }
> + }
> + free(zero_buf);
> + free(buf);
> +
> + if (wiped)
> + MSG(0, "Info: Found all zeros in first %d blocks\n", nblocks);
> + return wiped;
> +#else
> + return false;
> +#endif
> +}
> +
>  int f2fs_trim_devices(void)
>  {
>   int i;
>  
> - for (i = 0; i < c.ndevs; i++)
> - if (trim_device(i))
> + for (i = 0; i < c.ndevs; i++) {
> + if (!is_wiped_device(i) && trim_device(i))
>   return -1;
> + }
>   c.trimmed = 1;
>   return 0;
>  }
>
>>> .
>>>
> .
> 


___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel


Re: [f2fs-dev] [PATCH v2] mkfs.f2fs: check zeros in first 16MB for Android

2019-08-15 Thread Chao Yu
On 2019/8/16 6:21, Jaegeuk Kim wrote:
> On 08/12, Chao Yu wrote:
>> On 2019/8/9 23:12, Jaegeuk Kim wrote:
>>> We actually don't need to issue trim on entire disk by checking first
>>> blocks having zeros.
>>
>> In heap mode, we locate node log header to tail end of device, should we
>> consider to check block contain according to heap option?
> 
> I wanted to check F2FS metadata mainly.

Oh, I thought you mean main area. :P

> 
>>
>> BTW, if we changed cp_ver whenever mkfs, why should we still issue trim to
>> obsolete old data in node remained in image?
> 
> For simplicity. :P

I didn't get why we can assume all metadata are zeroed if first 16MB are all 
zero...

BTW, if first 16MB are non-zero, why not just trim F2FS metadata rather than
whole area?

Thanks,

> 
>>
>> Thanks,
>>
>>>
>>> Signed-off-by: Jaegeuk Kim 
>>> ---
>>> v2 from v1:
>>>  - clean up
>>>
>>>  mkfs/f2fs_format_utils.c | 53 ++--
>>>  1 file changed, 51 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/mkfs/f2fs_format_utils.c b/mkfs/f2fs_format_utils.c
>>> index 8bf128c..f2d55ad 100644
>>> --- a/mkfs/f2fs_format_utils.c
>>> +++ b/mkfs/f2fs_format_utils.c
>>> @@ -25,6 +25,7 @@
>>>  #include 
>>>  #include 
>>>  #include 
>>> +#include 
>>>  #ifndef ANDROID_WINDOWS_HOST
>>>  #include 
>>>  #endif
>>> @@ -110,13 +111,61 @@ static int trim_device(int i)
>>> return 0;
>>>  }
>>>  
>>> +static bool is_wiped_device(int i)
>>> +{
>>> +#ifdef WITH_ANDROID
>>> +   struct device_info *dev = c.devices + i;
>>> +   int fd = dev->fd;
>>> +   char *buf, *zero_buf;
>>> +   bool wiped = true;
>>> +   int nblocks = 4096; /* 16MB size */
>>> +   int j;
>>> +
>>> +   buf = malloc(F2FS_BLKSIZE);
>>> +   if (buf == NULL) {
>>> +   MSG(1, "\tError: Malloc Failed for buf!!!\n");
>>> +   return false;
>>> +   }
>>> +   zero_buf = calloc(1, F2FS_BLKSIZE);
>>> +   if (zero_buf == NULL) {
>>> +   MSG(1, "\tError: Calloc Failed for zero buf!!!\n");
>>> +   free(buf);
>>> +   return false;
>>> +   }
>>> +
>>> +   if (lseek(fd, 0, SEEK_SET) < 0) {
>>> +   free(zero_buf);
>>> +   free(buf);
>>> +   return false;
>>> +   }
>>> +
>>> +   /* check first n blocks */
>>> +   for (j = 0; j < nblocks; j++) {
>>> +   if (read(fd, buf, F2FS_BLKSIZE) != F2FS_BLKSIZE ||
>>> +   memcmp(buf, zero_buf, F2FS_BLKSIZE)) {
>>> +   wiped = false;
>>> +   break;
>>> +   }
>>> +   }
>>> +   free(zero_buf);
>>> +   free(buf);
>>> +
>>> +   if (wiped)
>>> +   MSG(0, "Info: Found all zeros in first %d blocks\n", nblocks);
>>> +   return wiped;
>>> +#else
>>> +   return false;
>>> +#endif
>>> +}
>>> +
>>>  int f2fs_trim_devices(void)
>>>  {
>>> int i;
>>>  
>>> -   for (i = 0; i < c.ndevs; i++)
>>> -   if (trim_device(i))
>>> +   for (i = 0; i < c.ndevs; i++) {
>>> +   if (!is_wiped_device(i) && trim_device(i))
>>> return -1;
>>> +   }
>>> c.trimmed = 1;
>>> return 0;
>>>  }
>>>
> .
> 


___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel


Re: [f2fs-dev] [PATCH v2] mkfs.f2fs: check zeros in first 16MB for Android

2019-08-15 Thread Jaegeuk Kim
On 08/16, Chao Yu wrote:
> On 2019/8/16 6:21, Jaegeuk Kim wrote:
> > On 08/12, Chao Yu wrote:
> >> On 2019/8/9 23:12, Jaegeuk Kim wrote:
> >>> We actually don't need to issue trim on entire disk by checking first
> >>> blocks having zeros.
> >>
> >> In heap mode, we locate node log header to tail end of device, should we
> >> consider to check block contain according to heap option?
> > 
> > I wanted to check F2FS metadata mainly.
> 
> Oh, I thought you mean main area. :P
> 
> > 
> >>
> >> BTW, if we changed cp_ver whenever mkfs, why should we still issue trim to
> >> obsolete old data in node remained in image?
> > 
> > For simplicity. :P
> 
> I didn't get why we can assume all metadata are zeroed if first 16MB are all 
> zero...
> 
> BTW, if first 16MB are non-zero, why not just trim F2FS metadata rather than
> whole area?

Trim the entire space so that we can skip discard in runtime by the flag, right?

> 
> Thanks,
> 
> > 
> >>
> >> Thanks,
> >>
> >>>
> >>> Signed-off-by: Jaegeuk Kim 
> >>> ---
> >>> v2 from v1:
> >>>  - clean up
> >>>
> >>>  mkfs/f2fs_format_utils.c | 53 ++--
> >>>  1 file changed, 51 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/mkfs/f2fs_format_utils.c b/mkfs/f2fs_format_utils.c
> >>> index 8bf128c..f2d55ad 100644
> >>> --- a/mkfs/f2fs_format_utils.c
> >>> +++ b/mkfs/f2fs_format_utils.c
> >>> @@ -25,6 +25,7 @@
> >>>  #include 
> >>>  #include 
> >>>  #include 
> >>> +#include 
> >>>  #ifndef ANDROID_WINDOWS_HOST
> >>>  #include 
> >>>  #endif
> >>> @@ -110,13 +111,61 @@ static int trim_device(int i)
> >>>   return 0;
> >>>  }
> >>>  
> >>> +static bool is_wiped_device(int i)
> >>> +{
> >>> +#ifdef WITH_ANDROID
> >>> + struct device_info *dev = c.devices + i;
> >>> + int fd = dev->fd;
> >>> + char *buf, *zero_buf;
> >>> + bool wiped = true;
> >>> + int nblocks = 4096; /* 16MB size */
> >>> + int j;
> >>> +
> >>> + buf = malloc(F2FS_BLKSIZE);
> >>> + if (buf == NULL) {
> >>> + MSG(1, "\tError: Malloc Failed for buf!!!\n");
> >>> + return false;
> >>> + }
> >>> + zero_buf = calloc(1, F2FS_BLKSIZE);
> >>> + if (zero_buf == NULL) {
> >>> + MSG(1, "\tError: Calloc Failed for zero buf!!!\n");
> >>> + free(buf);
> >>> + return false;
> >>> + }
> >>> +
> >>> + if (lseek(fd, 0, SEEK_SET) < 0) {
> >>> + free(zero_buf);
> >>> + free(buf);
> >>> + return false;
> >>> + }
> >>> +
> >>> + /* check first n blocks */
> >>> + for (j = 0; j < nblocks; j++) {
> >>> + if (read(fd, buf, F2FS_BLKSIZE) != F2FS_BLKSIZE ||
> >>> + memcmp(buf, zero_buf, F2FS_BLKSIZE)) {
> >>> + wiped = false;
> >>> + break;
> >>> + }
> >>> + }
> >>> + free(zero_buf);
> >>> + free(buf);
> >>> +
> >>> + if (wiped)
> >>> + MSG(0, "Info: Found all zeros in first %d blocks\n", nblocks);
> >>> + return wiped;
> >>> +#else
> >>> + return false;
> >>> +#endif
> >>> +}
> >>> +
> >>>  int f2fs_trim_devices(void)
> >>>  {
> >>>   int i;
> >>>  
> >>> - for (i = 0; i < c.ndevs; i++)
> >>> - if (trim_device(i))
> >>> + for (i = 0; i < c.ndevs; i++) {
> >>> + if (!is_wiped_device(i) && trim_device(i))
> >>>   return -1;
> >>> + }
> >>>   c.trimmed = 1;
> >>>   return 0;
> >>>  }
> >>>
> > .
> > 


___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel


Re: [f2fs-dev] [PATCH v2] f2fs_io: add get/setflags

2019-08-15 Thread Jaegeuk Kim
Signed-off-by: Jaegeuk Kim 
---
- Add + flags for now

 tools/f2fs_io/f2fs_io.c | 98 +
 tools/f2fs_io/f2fs_io.h | 14 ++
 2 files changed, 112 insertions(+)

diff --git a/tools/f2fs_io/f2fs_io.c b/tools/f2fs_io/f2fs_io.c
index a5a9836..add40c4 100644
--- a/tools/f2fs_io/f2fs_io.c
+++ b/tools/f2fs_io/f2fs_io.c
@@ -45,6 +45,102 @@ struct cmd_desc {
int cmd_flags;
 };
 
+#define getflags_desc "getflags ioctl"
+#define getflags_help  \
+"f2fs_io getflags [file]\n\n"  \
+"get a flag given the file\n"  \
+"flag can show \n" \
+"  casefold\n" \
+"  compression\n"  \
+"  nocompression\n"
+
+static void do_getflags(int argc, char **argv, const struct cmd_desc *cmd)
+{
+   long flag;
+   int ret, fd;
+   int exist = 0;
+
+   if (argc != 2) {
+   fputs("Excess arguments\n\n", stderr);
+   fputs(cmd->cmd_help, stderr);
+   exit(1);
+   }
+
+   fd = open(argv[1], O_RDONLY);
+   if (fd == -1) {
+   fputs("Open failed\n\n", stderr);
+   fputs(cmd->cmd_help, stderr);
+   exit(1);
+   }
+
+   ret = ioctl(fd, F2FS_IOC_GETFLAGS, );
+   printf("get a flag on %s ret=%d, flags=", argv[1], ret);
+   if (flag & FS_CASEFOLD_FL) {
+   printf("casefold");
+   exist = 1;
+   }
+   if (flag & FS_COMPR_FL) {
+   if (exist)
+   printf(",");
+   printf("compression");
+   exist = 1;
+   }
+   if (flag & FS_NOCOMP_FL) {
+   if (exist)
+   printf(",");
+   printf("nocompression");
+   exist = 1;
+   }
+   if (!exist)
+   printf("none");
+   printf("\n");
+   exit(0);
+}
+
+#define setflags_desc "setflags ioctl"
+#define setflags_help  \
+"f2fs_io setflags [flag] [file]\n\n"   \
+"set a flag given the file\n"  \
+"flag can be\n"\
+"  casefold\n" \
+"  compression\n"  \
+"  nocompression\n"
+
+static void do_setflags(int argc, char **argv, const struct cmd_desc *cmd)
+{
+   long flag;
+   int ret, fd;
+
+   if (argc != 3) {
+   fputs("Excess arguments\n\n", stderr);
+   fputs(cmd->cmd_help, stderr);
+   exit(1);
+   }
+
+   fd = open(argv[2], O_RDONLY);
+   if (fd == -1) {
+   fputs("Open failed\n\n", stderr);
+   fputs(cmd->cmd_help, stderr);
+   exit(1);
+   }
+
+   ret = ioctl(fd, F2FS_IOC_GETFLAGS, );
+   printf("get a flag on %s ret=%d, flags=%lx\n", argv[1], ret, flag);
+   if (ret)
+   exit(1);
+
+   if (!strcmp(argv[1], "casefold"))
+   flag |= FS_CASEFOLD_FL;
+   else if (!strcmp(argv[1], "compression"))
+   flag |= FS_COMPR_FL;
+   else if (!strcmp(argv[1], "nocompression"))
+   flag |= FS_NOCOMP_FL;
+
+   ret = ioctl(fd, F2FS_IOC_SETFLAGS, );
+   printf("set a flag on %s ret=%d, flags=%s\n", argv[2], ret, argv[1]);
+   exit(0);
+}
+
 #define shutdown_desc "shutdown filesystem"
 #define shutdown_help  \
 "f2fs_io shutdown [level] [dir]\n\n"   \
@@ -488,6 +584,8 @@ static void do_defrag_file(int argc, char **argv, const 
struct cmd_desc *cmd)
 static void do_help(int argc, char **argv, const struct cmd_desc *cmd);
 const struct cmd_desc cmd_list[] = {
_CMD(help),
+   CMD(getflags),
+   CMD(setflags),
CMD(shutdown),
CMD(pinfile),
CMD(fallocate),
diff --git a/tools/f2fs_io/f2fs_io.h b/tools/f2fs_io/f2fs_io.h
index 95fd5be..5768c1b 100644
--- a/tools/f2fs_io/f2fs_io.h
+++ b/tools/f2fs_io/f2fs_io.h
@@ -41,9 +41,13 @@ typedef u32  __be32;
 #ifndef FS_IOC_GETFLAGS
 #define FS_IOC_GETFLAGS_IOR('f', 1, long)
 #endif
+#ifndef FS_IOC_SETFLAGS
+#define FS_IOC_SETFLAGS_IOW('f', 2, long)
+#endif
 
 #define F2FS_IOCTL_MAGIC   0xf5
 #define F2FS_IOC_GETFLAGS  FS_IOC_GETFLAGS
+#define F2FS_IOC_SETFLAGS  FS_IOC_SETFLAGS
 
 #define F2FS_IOC_START_ATOMIC_WRITE_IO(F2FS_IOCTL_MAGIC, 1)
 #define F2FS_IOC_COMMIT_ATOMIC_WRITE   _IO(F2FS_IOCTL_MAGIC, 2)
@@ -98,6 +102,16 @@ typedef u32 __be32;
 #define F2FS_IOC_FSGETXATTRFS_IOC_FSGETXATTR
 #define F2FS_IOC_FSSETXATTRFS_IOC_FSSETXATTR
 
+#ifndef FS_NOCOMP_FL
+#define FS_NOCOMP_FL   0x0400 /* Don't 

Re: [f2fs-dev] [PATCH 2/4] fsck.f2fs: fix to check c.fix_on before repair

2019-08-15 Thread Chao Yu
On 2019/8/16 9:06, Jaegeuk Kim wrote:
> On 08/12, Chao Yu wrote:
>> We should always set c.bug_on whenever found a bug, then fix them
>> if c.fix_on is on, otherwise, some bugs won't be shown unless we
>> enable debug log.
>>
>> Signed-off-by: Chao Yu 
>> ---
>>  fsck/fsck.c | 137 +++-
>>  1 file changed, 83 insertions(+), 54 deletions(-)
>>
>> diff --git a/fsck/fsck.c b/fsck/fsck.c
>> index 7eb599d..91ddd49 100644
>> --- a/fsck/fsck.c
>> +++ b/fsck/fsck.c
>> @@ -712,12 +712,13 @@ void fsck_chk_inode_blk(struct f2fs_sb_info *sbi, u32 
>> nid,
>>  fsck_reada_node_block(sbi, le32_to_cpu(node_blk->i.i_xattr_nid));
>>  
>>  if (fsck_chk_xattr_blk(sbi, nid,
>> -le32_to_cpu(node_blk->i.i_xattr_nid), blk_cnt) &&
>> -c.fix_on) {
>> -node_blk->i.i_xattr_nid = 0;
>> -need_fix = 1;
>> -FIX_MSG("Remove xattr block: 0x%x, x_nid = 0x%x",
>> -nid, le32_to_cpu(node_blk->i.i_xattr_nid));
>> +le32_to_cpu(node_blk->i.i_xattr_nid), blk_cnt)) {
>> +if (c.fix_on) {
>> +node_blk->i.i_xattr_nid = 0;
>> +need_fix = 1;
>> +FIX_MSG("Remove xattr block: 0x%x, x_nid = 0x%x",
>> +nid, 
>> le32_to_cpu(node_blk->i.i_xattr_nid));
>> +}
> 
> Why do we need this change?

Actually, it's not necessary, but just cleanup to keep the same style. :P

Thanks,

> 
>>  }
>>  
>>  if (ftype == F2FS_FT_CHRDEV || ftype == F2FS_FT_BLKDEV ||
>> @@ -730,24 +731,32 @@ void fsck_chk_inode_blk(struct f2fs_sb_info *sbi, u32 
>> nid,
>>  
>>  if (f2fs_has_extra_isize(_blk->i)) {
>>  if (c.feature & cpu_to_le32(F2FS_FEATURE_EXTRA_ATTR)) {
>> -if (node_blk->i.i_extra_isize >
>> -cpu_to_le16(F2FS_TOTAL_EXTRA_ATTR_SIZE)) {
>> -FIX_MSG("ino[0x%x] recover i_extra_isize "
>> -"from %u to %u",
>> -nid,
>> -le16_to_cpu(node_blk->i.i_extra_isize),
>> -calc_extra_isize());
>> -node_blk->i.i_extra_isize =
>> -cpu_to_le16(calc_extra_isize());
>> -need_fix = 1;
>> +unsigned int isize =
>> +le16_to_cpu(node_blk->i.i_extra_isize);
>> +
>> +if (isize > F2FS_TOTAL_EXTRA_ATTR_SIZE) {
>> +ASSERT_MSG("[0x%x] wrong i_extra_isize=0x%x",
>> +nid, isize);
>> +if (c.fix_on) {
>> +FIX_MSG("ino[0x%x] recover 
>> i_extra_isize "
>> +"from %u to %u",
>> +nid, isize,
>> +calc_extra_isize());
>> +node_blk->i.i_extra_isize =
>> +cpu_to_le16(calc_extra_isize());
>> +need_fix = 1;
>> +}
>>  }
>>  } else {
>> -FIX_MSG("ino[0x%x] remove F2FS_EXTRA_ATTR "
>> -"flag in i_inline:%u",
>> -nid, node_blk->i.i_inline);
>> -/* we don't support tuning F2FS_FEATURE_EXTRA_ATTR now 
>> */
>> -node_blk->i.i_inline &= ~F2FS_EXTRA_ATTR;
>> -need_fix = 1;
>> +ASSERT_MSG("[0x%x] wrong extra_attr flag", nid);
>> +if (c.fix_on) {
>> +FIX_MSG("ino[0x%x] remove F2FS_EXTRA_ATTR "
>> +"flag in i_inline:%u",
>> +nid, node_blk->i.i_inline);
>> +/* we don't support tuning 
>> F2FS_FEATURE_EXTRA_ATTR now */
>> +node_blk->i.i_inline &= ~F2FS_EXTRA_ATTR;
>> +need_fix = 1;
>> +}
>>  }
>>  
>>  if ((c.feature &
>> @@ -758,13 +767,17 @@ void fsck_chk_inode_blk(struct f2fs_sb_info *sbi, u32 
>> nid,
>>  
>>  if (!inline_size ||
>>  inline_size > MAX_INLINE_XATTR_SIZE) {
>> -FIX_MSG("ino[0x%x] recover inline xattr size "
>> -"from %u to %u",
>> -nid, inline_size,
>> -DEFAULT_INLINE_XATTR_ADDRS);
>> -node_blk->i.i_inline_xattr_size =
>> -

Re: [f2fs-dev] [PATCH] f2fs_io: add get/setflags

2019-08-15 Thread Jaegeuk Kim
On 08/15, Jaegeuk Kim wrote:
> On 08/12, Chao Yu wrote:
> > On 2019/8/9 23:23, Jaegeuk Kim wrote:
> > > From: Jaegeuk Kim 
> > > 
> > > Signed-off-by: Jaegeuk Kim 
> > > ---
> > >  tools/f2fs_io/f2fs_io.c | 91 +
> > >  tools/f2fs_io/f2fs_io.h | 14 +++
> > >  2 files changed, 105 insertions(+)
> > > 
> > > diff --git a/tools/f2fs_io/f2fs_io.c b/tools/f2fs_io/f2fs_io.c
> > > index 6b43778..b57c458 100644
> > > --- a/tools/f2fs_io/f2fs_io.c
> > > +++ b/tools/f2fs_io/f2fs_io.c
> > > @@ -45,6 +45,95 @@ struct cmd_desc {
> > >   int cmd_flags;
> > >  };
> > >  
> > > +#define getflags_desc "getflags ioctl"
> > > +#define getflags_help\
> > > +"f2fs_io getflags [file]\n\n"\
> > > +"get a flag given the file\n"\
> > > +"flag can show \n"   \
> > > +"  casefold\n"   \
> > > +"  compression\n"\
> > > +"  nocompression\n"
> > 
> > Could we support +/- flags?
> > 
> > > +
> > > +static void do_getflags(int argc, char **argv, const struct cmd_desc 
> > > *cmd)
> > > +{
> > > + long flag;
> > > + int ret, fd;
> > > + int exist = 0;
> > > +
> > > + if (argc != 2) {
> > > + fputs("Excess arguments\n\n", stderr);
> > > + fputs(cmd->cmd_help, stderr);
> > > + exit(1);
> > > + }
> > > +
> > > + fd = open(argv[1], O_RDONLY);
> > > + if (fd == -1) {
> > > + fputs("Open failed\n\n", stderr);
> > > + fputs(cmd->cmd_help, stderr);
> > > + exit(1);
> > > + }
> > > + ret = ioctl(fd, F2FS_IOC_GETFLAGS, );
> > > + printf("get a flag on %s ret=%d, flags=", argv[1], ret);
> > > + if (flag & FS_CASEFOLD_FL) {
> > > + printf("casefold");
> > > + exist = 1;
> > > + }
> > > + if (flag & FS_COMPR_FL) {
> > > + if (exist)
> > > + printf(",");
> > > + printf("compression");
> > > + exist = 1;
> > > + }
> > > + if (flag & FS_NOCOMP_FL) {
> > > + if (exist)
> > > + printf(",");
> > > + printf("nocompression");
> > > + exist = 1;
> > > + }
> > > + if (!exist)
> > > + printf("none");
> > > + printf("\n");
> > > + exit(0);
> > > +}
> > > +
> > > +#define setflags_desc "setflags ioctl"
> > > +#define setflags_help\
> > > +"f2fs_io setflags [flag] [file]\n\n" \
> > > +"set a flag given the file\n"\
> > > +"flag can be\n"  \
> > > +"  casefold\n"   \
> > > +"  compression\n"\
> > > +"  nocompression\n"
> > > +
> > > +static void do_setflags(int argc, char **argv, const struct cmd_desc 
> > > *cmd)
> > > +{
> > > + long flag;
> > > + int ret, fd;
> > > +
> > > + if (argc != 3) {
> > > + fputs("Excess arguments\n\n", stderr);
> > > + fputs(cmd->cmd_help, stderr);
> > > + exit(1);
> > > + }
> > > +
> > > + fd = open(argv[2], O_RDONLY);
> > > + if (fd == -1) {
> > > + fputs("Open failed\n\n", stderr);
> > > + fputs(cmd->cmd_help, stderr);
> > > + exit(1);
> > > + }
> > > + if (!strcmp(argv[1], "casefold"))
> > > + flag = FS_CASEFOLD_FL;
> > > + else if (!strcmp(argv[1], "compression"))
> > > + flag = FS_COMPR_FL;
> > > + else if (!strcmp(argv[1], "nocompression"))
> > > + flag = FS_NOCOMP_FL;
> > > +
> > > + ret = ioctl(fd, F2FS_IOC_SETFLAGS, );
> > 
> > It will drop other existed flags, should be:
> 
> Is it correct?
> 
> In f2fs_setflags_common,
> 
>   fi->i_flags = iflags | (fi->i_flags & ~mask);

Hmm, it indeed drops the flags. :P

> 
> 
> > 
> > F2FS_IOC_GETFLAGS flag
> > flag +/- FS_*_FL
> > F2FS_IOC_SETFLAGS flag
> > 
> > Thanks,
> > 
> > > + printf("set a flag: %s ret=%d\n", argv[2], ret);
> > > + exit(0);
> > > +}
> > > +
> > >  #define shutdown_desc "shutdown filesystem"
> > >  #define shutdown_help\
> > >  "f2fs_io shutdown [level] [dir]\n\n" \
> > > @@ -488,6 +577,8 @@ static void do_defrag_file(int argc, char **argv, 
> > > const struct cmd_desc *cmd)
> > >  static void do_help(int argc, char **argv, const struct cmd_desc *cmd);
> > >  const struct cmd_desc cmd_list[] = {
> > >   _CMD(help),
> > > + CMD(getflags),
> > > + CMD(setflags),
> > >   CMD(shutdown),
> > >   CMD(pinfile),
> > >   CMD(fallocate),
> > > diff --git a/tools/f2fs_io/f2fs_io.h b/tools/f2fs_io/f2fs_io.h
> > > index 95fd5be..5768c1b 100644
> > > --- a/tools/f2fs_io/f2fs_io.h
> > > +++ b/tools/f2fs_io/f2fs_io.h
> > > @@ -41,9 +41,13 @@ typedef u32__be32;
> > >  #ifndef FS_IOC_GETFLAGS
> > >  #define FS_IOC_GETFLAGS  _IOR('f', 1, long)

[PATCH] f2fs: fix to avoid data corruption by forbidding SSR overwrite

2019-08-15 Thread Chao Yu
There is one case can cause data corruption.

- write 4k to fileA
- fsync fileA, 4k data is writebacked to lbaA
- write 4k to fileA
- kworker flushs 4k to lbaB; dnode contain lbaB didn't be persisted yet
- write 4k to fileB
- kworker flush 4k to lbaA due to SSR
- SPOR -> dnode with lbaA will be recovered, however lbaA contains fileB's
data

One solution is tracking all fsynced file's block history, and disallow
SSR overwrite on newly invalidated block on that file.

However, during recovery, no matter the dnode is flushed or fsynced, all
previous dnodes until last fsynced one in node chain can be recovered,
that means we need to record all block change in flushed dnode, which
will cause heavy cost, so let's just use simple fix by forbidding SSR
overwrite directly.

Signed-off-by: Chao Yu 
---
 fs/f2fs/segment.c | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index 9d9d9a050d59..69b3b553ee6b 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -2205,9 +2205,11 @@ static void update_sit_entry(struct f2fs_sb_info *sbi, 
block_t blkaddr, int del)
if (!f2fs_test_and_set_bit(offset, se->discard_map))
sbi->discard_blks--;
 
-   /* don't overwrite by SSR to keep node chain */
-   if (IS_NODESEG(se->type) &&
-   !is_sbi_flag_set(sbi, SBI_CP_DISABLED)) {
+   /*
+* SSR should never reuse block which is checkpointed
+* or newly invalidated.
+*/
+   if (!is_sbi_flag_set(sbi, SBI_CP_DISABLED)) {
if (!f2fs_test_and_set_bit(offset, se->ckpt_valid_map))
se->ckpt_valid_blocks++;
}
-- 
2.18.0.rc1



[f2fs-dev] [PATCH 0/3] f2fs: fixes for FS_IOC_{GET,SET}FSLABEL

2019-08-15 Thread Eric Biggers
Fix some bugs in the f2fs implementation of the FS_IOC_GETFSLABEL and
FS_IOC_SETFSLABEL ioctls.

Eric Biggers (3):
  f2fs: fix buffer overruns in FS_IOC_{GET,SET}FSLABEL
  f2fs: fix copying too many bytes in FS_IOC_SETFSLABEL
  f2fs: add missing authorization check in FS_IOC_SETFSLABEL

 fs/f2fs/file.c | 27 +--
 1 file changed, 9 insertions(+), 18 deletions(-)

-- 
2.22.0



___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel


[f2fs-dev] [PATCH 1/3] f2fs: fix buffer overruns in FS_IOC_{GET, SET}FSLABEL

2019-08-15 Thread Eric Biggers
From: Eric Biggers 

utf16s_to_utf8s() and utf8s_to_utf16s() take the number of characters,
not the number of bytes.

Fixes: 61a3da4d5ef8 ("f2fs: support FS_IOC_{GET,SET}FSLABEL")
Signed-off-by: Eric Biggers 
---
 fs/f2fs/file.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index eb1aa9b75eda..d521a582d94d 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -3094,7 +3094,7 @@ static int f2fs_get_volume_name(struct file *filp, 
unsigned long arg)
 
down_read(>sb_lock);
count = utf16s_to_utf8s(sbi->raw_super->volume_name,
-   sizeof(sbi->raw_super->volume_name),
+   ARRAY_SIZE(sbi->raw_super->volume_name),
UTF16_LITTLE_ENDIAN, vbuf, MAX_VOLUME_NAME);
up_read(>sb_lock);
 
@@ -3139,7 +3139,7 @@ static int f2fs_set_volume_name(struct file *filp, 
unsigned long arg)
sizeof(sbi->raw_super->volume_name));
utf8s_to_utf16s(vbuf, MAX_VOLUME_NAME, UTF16_LITTLE_ENDIAN,
sbi->raw_super->volume_name,
-   sizeof(sbi->raw_super->volume_name));
+   ARRAY_SIZE(sbi->raw_super->volume_name));
 
err = f2fs_commit_super(sbi, false);
 
-- 
2.22.0



___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel


[f2fs-dev] [PATCH 3/3] f2fs: add missing authorization check in FS_IOC_SETFSLABEL

2019-08-15 Thread Eric Biggers
From: Eric Biggers 

FS_IOC_SETFSLABEL modifies the filesystem superblock, so it shouldn't be
allowed to regular users.  Require CAP_SYS_ADMIN, like xfs and btrfs do.

Fixes: 61a3da4d5ef8 ("f2fs: support FS_IOC_{GET,SET}FSLABEL")
Signed-off-by: Eric Biggers 
---
 fs/f2fs/file.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 315127251bc1..6939cddc542a 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -3113,6 +3113,9 @@ static int f2fs_set_volume_name(struct file *filp, 
unsigned long arg)
char *vbuf;
int err = 0;
 
+   if (!capable(CAP_SYS_ADMIN))
+   return -EPERM;
+
vbuf = strndup_user((const char __user *)arg, FSLABEL_MAX);
if (IS_ERR(vbuf))
return PTR_ERR(vbuf);
-- 
2.22.0



___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel


[f2fs-dev] [PATCH 2/3] f2fs: fix copying too many bytes in FS_IOC_SETFSLABEL

2019-08-15 Thread Eric Biggers
From: Eric Biggers 

Userspace provides a null-terminated string, so don't assume that the
full FSLABEL_MAX bytes can always be copied.

Fixes: 61a3da4d5ef8 ("f2fs: support FS_IOC_{GET,SET}FSLABEL")
Signed-off-by: Eric Biggers 
---
 fs/f2fs/file.c | 22 +-
 1 file changed, 5 insertions(+), 17 deletions(-)

diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index d521a582d94d..315127251bc1 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -3111,23 +3111,11 @@ static int f2fs_set_volume_name(struct file *filp, 
unsigned long arg)
struct inode *inode = file_inode(filp);
struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
char *vbuf;
-   int len;
int err = 0;
 
-   vbuf = f2fs_kzalloc(sbi, MAX_VOLUME_NAME, GFP_KERNEL);
-   if (!vbuf)
-   return -ENOMEM;
-
-   if (copy_from_user(vbuf, (char __user *)arg, FSLABEL_MAX)) {
-   err = -EFAULT;
-   goto out;
-   }
-
-   len = strnlen(vbuf, FSLABEL_MAX);
-   if (len > FSLABEL_MAX - 1) {
-   err = -EINVAL;
-   goto out;
-   }
+   vbuf = strndup_user((const char __user *)arg, FSLABEL_MAX);
+   if (IS_ERR(vbuf))
+   return PTR_ERR(vbuf);
 
err = mnt_want_write_file(filp);
if (err)
@@ -3137,7 +3125,7 @@ static int f2fs_set_volume_name(struct file *filp, 
unsigned long arg)
 
memset(sbi->raw_super->volume_name, 0,
sizeof(sbi->raw_super->volume_name));
-   utf8s_to_utf16s(vbuf, MAX_VOLUME_NAME, UTF16_LITTLE_ENDIAN,
+   utf8s_to_utf16s(vbuf, strlen(vbuf), UTF16_LITTLE_ENDIAN,
sbi->raw_super->volume_name,
ARRAY_SIZE(sbi->raw_super->volume_name));
 
@@ -3147,7 +3135,7 @@ static int f2fs_set_volume_name(struct file *filp, 
unsigned long arg)
 
mnt_drop_write_file(filp);
 out:
-   kvfree(vbuf);
+   kfree(vbuf);
return err;
 }
 
-- 
2.22.0



___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel