RE: [PATCH 30/32] Documentation: net: dsa: update configuration.rst reference
> Em Thu, 1 Apr 2021 14:41:15 +0200
> Andrew Lunn escreveu:
>
> > On Thu, Apr 01, 2021 at 02:17:50PM +0200, Mauro Carvalho Chehab wrote:
> > > The file name: Documentation/configuration.txt should be, instead:
> > > Documentation/networking/dsa/configuration.rst.
> > >
> > > Update its cross-reference accordingly.
> > >
> > > Fixes: 75b8988dfe83 ("cifsd: add server handler for central
> > > processing and tranport layers")
> > > Fixes: 58dd7a8d9d02 ("Documentation: net: dsa: Describe DSA switch
> > > configuration")
> > > Signed-off-by: Mauro Carvalho Chehab
> > > ---
> > > Documentation/filesystems/cifs/cifsd.rst | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/Documentation/filesystems/cifs/cifsd.rst
> > > b/Documentation/filesystems/cifs/cifsd.rst
> > > index 48ae58f2a53c..082a839535e7 100644
> > > --- a/Documentation/filesystems/cifs/cifsd.rst
> > > +++ b/Documentation/filesystems/cifs/cifsd.rst
> > > @@ -114,7 +114,7 @@ How to run
> > > # ksmbd.adduser -a
> > >
> > > 3. Create /etc/ksmbd/smb.conf file, add SMB share in smb.conf file
> > > - - Refer smb.conf.example and Documentation/configuration.txt
> > > + - Refer smb.conf.example and
> > > +Documentation/networking/dsa/configuration.rst
> > > in ksmbd-tools
> >
> > Hi Mauro
> >
> > This looks wrong. There is no relationship between SMB and DSA. I
> > suspect you are looking for some other configuration.txt
>
> Thanks for reviewing it.
>
> I'll drop the patch from my series.
>
> After re-reading cifsd.rst, I suspect that it is actually trying to refer to:
>
>
> https://protect2.fireeye.com/v1/url?k=28e6ada6-777d956f-28e726e9-0cc47a336fae-
> 3420a4e980f3c5ac=1=68e90996-b225-48b8-88cd-a32b7a3d23b6=https%3A%2F%2Fgithub.com%2Fcifsd-
> team%2Fksmbd-tools%2Fblob%2Fmaster%2FDocumentation%2Fconfiguration.txt
>
> and not to a local file.
>
> So, IMO, the right thing to do is to apply the enclosed patch instead.
ksmbd is still in review stage. not yet merged into linus tree.
So please don't include it in your series. I will apply the patch below
directly.
Thanks for your patch!
>
>
> Thanks,
> Mauro
>
> [PATCH] docs: cifsd: change the reference to configuration.txt
>
> Changeset 75b8988dfe83 ("cifsd: add server handler for central processing and
> tranport layers") added
> documentation for cifsd. There, it points to a file
> named:
> Documentation/configuration.txt
>
> This confuses Kernel scripts, as they think that this is a document within
> the Kernel tree, instead of
> a file from some other place.
>
> Replace it by an hyperlink to the ksmbd-tools tree, in order to avoid
> false-positives.
>
> Signed-off-by: Mauro Carvalho Chehab
>
> diff --git a/Documentation/filesystems/cifs/cifsd.rst
> b/Documentation/filesystems/cifs/cifsd.rst
> index 48ae58f2a53c..a6ab85e68252 100644
> --- a/Documentation/filesystems/cifs/cifsd.rst
> +++ b/Documentation/filesystems/cifs/cifsd.rst
> @@ -114,8 +114,8 @@ How to run
> # ksmbd.adduser -a
>
> 3. Create /etc/ksmbd/smb.conf file, add SMB share in smb.conf file
> - - Refer smb.conf.example and Documentation/configuration.txt
> - in ksmbd-tools
> + - Refer smb.conf.example and
> +
> +https://protect2.fireeye.com/v1/url?k=6eb727bd-312c1f74-6eb6acf2-0cc47a
> +336fae-b5ccd9ba5c41390c=1=68e90996-b225-48b8-88cd-a32b7a3d23b6=ht
> +tps%3A%2F%2Fgithub.com%2Fcifsd-team%2Fksmbd-tools%2Fblob%2Fmaster%2FDoc
> +umentation%2Fconfiguration.txt
>
> 4. Insert ksmbd.ko module
>
>
Re: [Linux-cifsd-devel] [PATCH] cifsd: use kfree to free memory allocated by kzalloc
2021-04-01 22:14 GMT+09:00, Ralph Boehme :
> Am 4/1/21 um 2:59 PM schrieb Namjae Jeon:
>> 2021-04-01 21:50 GMT+09:00, Ralph Boehme :
>>> fwiw, while at it what about renaming everything that still references
>>> "cifs" to "smb" ? This is not the 90's... :)
>> It is also used with the name "ksmbd". So function and variable prefix
>> are used with ksmbd.
>
> well, I was thinking of this:
>
> > +++ b/fs/cifsd/...
>
> We should really stop using the name cifs for modern implementation of
> SMB{23} and the code should not be added as fs/cifsd/ to the kernel.
As I know, currently "cifs" is being used for the subdirectory name
for historical reasons and to avoid confusions, even though the CIFS
(SMB1) dialect is no longer recommended.
>
> Cheers!
> -slow
>
> --
> Ralph Boehme, Samba Teamhttps://samba.org/
> Samba Developer, SerNet GmbH https://sernet.de/en/samba/
> GPG-Fingerprint FAE2C6088A24252051C559E4AA1E9B7126399E46
>
>
Re: [Linux-cifsd-devel] [PATCH] cifsd: use kfree to free memory allocated by kzalloc
2021-04-01 20:50 GMT+09:00, Dan Carpenter :
> On Thu, Apr 01, 2021 at 04:39:33PM +0500, Muhammad Usama Anjum wrote:
>> kfree should be used to free memory allocated by kzalloc to avoid
>> any overhead and for maintaining consistency.
>>
>> Fixes: 5dfeb6d945 ("cifsd: use kmalloc() for small allocations")
>> Signed-off-by: Muhammad Usama Anjum
>> ---
>> This one place was left in earlier patch. I've already received
>> responsse on that patch. I'm sending a separate patch.
>>
>> fs/cifsd/transport_tcp.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/fs/cifsd/transport_tcp.c b/fs/cifsd/transport_tcp.c
>> index 67163efcf472..040881893417 100644
>> --- a/fs/cifsd/transport_tcp.c
>> +++ b/fs/cifsd/transport_tcp.c
>> @@ -551,7 +551,7 @@ void ksmbd_tcp_destroy(void)
>> list_for_each_entry_safe(iface, tmp, _list, entry) {
>> list_del(>entry);
>> kfree(iface->name);
>> -ksmbd_free(iface);
>> +kfree(iface);
>
> We should just delete the ksmbd_free() function completely.
Yes, I have added your review comment about this to my todo-list.
I will do that.
>
> I think that cifsd is being re-written though so it might not be worth
> it.
Right.
Thanks!
>
> regards,
> dan carpenter
>
>
> ___
> Linux-cifsd-devel mailing list
> linux-cifsd-de...@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-cifsd-devel
>
Re: [Linux-cifsd-devel] [PATCH] cifsd: use kfree to free memory allocated by kzalloc
2021-04-01 21:50 GMT+09:00, Ralph Boehme :
> Am 4/1/21 um 2:43 PM schrieb Namjae Jeon:
>> 2021-04-01 20:50 GMT+09:00, Dan Carpenter :
>>> On Thu, Apr 01, 2021 at 04:39:33PM +0500, Muhammad Usama Anjum wrote:
>>>> kfree should be used to free memory allocated by kzalloc to avoid
>>>> any overhead and for maintaining consistency.
>>>>
>>>> Fixes: 5dfeb6d945 ("cifsd: use kmalloc() for small allocations")
>>>> Signed-off-by: Muhammad Usama Anjum
>>>> ---
>>>> This one place was left in earlier patch. I've already received
>>>> responsse on that patch. I'm sending a separate patch.
>>>>
>>>> fs/cifsd/transport_tcp.c | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/fs/cifsd/transport_tcp.c b/fs/cifsd/transport_tcp.c
>>>> index 67163efcf472..040881893417 100644
>>>> --- a/fs/cifsd/transport_tcp.c
>>>> +++ b/fs/cifsd/transport_tcp.c
>>>> @@ -551,7 +551,7 @@ void ksmbd_tcp_destroy(void)
>>>>list_for_each_entry_safe(iface, tmp, _list, entry) {
>>>>list_del(>entry);
>>>>kfree(iface->name);
>>>> - ksmbd_free(iface);
>>>> + kfree(iface);
>>>
>>> We should just delete the ksmbd_free() function completely.
>> Yes, I have added your review comment about this to my todo-list.
>> I will do that.
>>>
>>> I think that cifsd is being re-written though so it might not be worth
>>> it.
>> Right.
>
> fwiw, while at it what about renaming everything that still references
> "cifs" to "smb" ? This is not the 90's... :)
It is also used with the name "ksmbd". So function and variable prefix
are used with ksmbd.
Thanks!
>
> Cheers!
> -slow
>
>
RE: [PATCH] cifsd: use kfree to free memory allocated by kmalloc or kzalloc
>
> kfree should be used to free memory allocated by kmalloc or kzalloc to avoid
> any overhead and for
> maintaining consistency.
>
> Fixes: 5dfeb6d945 ("cifsd: use kmalloc() for small allocations")
> Signed-off-by: Muhammad Usama Anjum
Looks good. I will apply. Thanks for your patch!
RE: [PATCH] cifsd: fix memory leak when loop ends
> > Memory is being allocated and if veto_list is zero, the loop breaks without > cleaning up the allocated > memory. In this patch, the length check has been moved before allocation. If > loop breaks, the memory > isn't allocated in the first place. Thus the memory is being protected from > leaking. > > Signed-off-by: Muhammad Usama Anjum I will apply. Thanks for your patch!
Re: [Linux-cifsd-devel] [PATCH][next] cifsd: remove redundant assignment to variable err
2021-03-26 2:35 GMT+09:00, Colin King :
> From: Colin Ian King
>
> The variable err is being initialized with a value that is never read
> and it is being updated later with a new value. The initialization is
> redundant and can be removed.
>
> Addresses-Coverity: ("Unused value")
> Signed-off-by: Colin Ian King
I will apply. Thanks for your patch!
RE: [PATCH v3] exfat: speed up iterate/lookup by fixing start point of traversing cluster chain
> > When directory iterate and lookup is called, there's a buggy rewinding > > of start point for traversing cluster chain to the parent directory > > entry's first cluster. This caused repeated cluster chain traversing > > from the first entry of the parent directory that would show worse > > performance if huge amounts of files exist under the parent directory. > > Fix not to rewind, make continue from currently referenced cluster and > > dir entry. > > > > Tested with 50,000 files under single directory / 256GB sdcard, with > > command "time ls -l > /dev/null", > > Before : 0m08.69s real 0m00.27s user 0m05.91s system > > After : 0m07.01s real 0m00.25s user 0m04.34s system > > > > Signed-off-by: Hyeongseok Kim > > Looks good. > Thanks for your contribution. > > Reviewed-by: Sungjong Seo Applied. Thanks!
RE: [PATCH 1/5] cifsd: add server handler and tranport layers
> On Tue, Mar 23, 2021 at 12:01:22PM +0900, Namjae Jeon wrote: > > > On Mon, Mar 22, 2021 at 02:13:40PM +0900, Namjae Jeon wrote: > > > > +#define RESPONSE_BUF(w)((void *)(w)->response_buf) > > > > +#define REQUEST_BUF(w) ((void *)(w)->request_buf) > > > > > > Why do you do this obfuscation? > > I don't remember exactly, but back then, It looked easier... > > > > > > > +#define RESPONSE_BUF_NEXT(w) \ > > > > + ((void *)((w)->response_buf + (w)->next_smb2_rsp_hdr_off)) > > > > +#define REQUEST_BUF_NEXT(w)\ > > > > + ((void *)((w)->request_buf + (w)->next_smb2_rcv_hdr_off)) > > > > > > These obfuscations aren't even used; delete them > > They are used in many place. > > Oh, argh. patch 2/5 was too big, so it didn't make it into the mailing list > archive I was using to > try to review this series. Please break it up into smaller pieces for next > time! Okay:) Thanks!
RE: [PATCH 1/5] cifsd: add server handler and tranport layers
> On Mon, Mar 22, 2021 at 02:13:40PM +0900, Namjae Jeon wrote: > > +#define RESPONSE_BUF(w)((void *)(w)->response_buf) > > +#define REQUEST_BUF(w) ((void *)(w)->request_buf) > > Why do you do this obfuscation? I don't remember exactly, but back then, It looked easier... > > > +#define RESPONSE_BUF_NEXT(w) \ > > + ((void *)((w)->response_buf + (w)->next_smb2_rsp_hdr_off)) > > +#define REQUEST_BUF_NEXT(w)\ > > + ((void *)((w)->request_buf + (w)->next_smb2_rcv_hdr_off)) > > These obfuscations aren't even used; delete them They are used in many place. ./smb2pdu.c:*rsp = RESPONSE_BUF_NEXT(work); ./smb2pdu.c:err_rsp = RESPONSE_BUF_NEXT(work); ./smb2pdu.c:rsp_hdr = RESPONSE_BUF_NEXT(work); ./smb2pdu.c:struct smb2_hdr *hdr = RESPONSE_BUF_NEXT(work); ./smb2pdu.c:struct smb2_hdr *rsp = RESPONSE_BUF_NEXT(work); ./smb2pdu.c:rsp_hdr = RESPONSE_BUF_NEXT(work); ./smb2pdu.c:rsp = RESPONSE_BUF_NEXT(work); ./smb2pdu.c:rsp = RESPONSE_BUF_NEXT(work); ./smb2pdu.c:hdr = RESPONSE_BUF_NEXT(work); ./smb2pdu.c:hdr = RESPONSE_BUF_NEXT(work); ./smb2pdu.c:rsp = RESPONSE_BUF_NEXT(work); ./smb2pdu.c:*req = REQUEST_BUF_NEXT(work); ./smb2pdu.c:rcv_hdr = REQUEST_BUF_NEXT(work); ./smb2pdu.c:struct smb2_hdr *req_hdr = REQUEST_BUF_NEXT(work); ./smb2pdu.c:struct smb2_hdr *req = REQUEST_BUF_NEXT(work); ./smb2pdu.c:rcv_hdr = REQUEST_BUF_NEXT(work); ./smb2pdu.c:hdr = REQUEST_BUF_NEXT(work); ./smb2pdu.c:req = REQUEST_BUF_NEXT(work); ./smb2pdu.c:req = REQUEST_BUF_NEXT(work); ./smb2pdu.c:hdr = REQUEST_BUF_NEXT(work); ./smb2pdu.c:req_hdr = REQUEST_BUF_NEXT(work); ./smb2pdu.c:hdr = REQUEST_BUF_NEXT(work); ./smb2pdu.c:req_hdr = REQUEST_BUF_NEXT(work); > > > +#define RESPONSE_SZ(w) ((w)->response_sz) > > + > > +#define INIT_AUX_PAYLOAD(w)((w)->aux_payload_buf = NULL) > > +#define HAS_AUX_PAYLOAD(w) ((w)->aux_payload_sz != 0) > > I mean, do you really find it clearer to write: > > if (HAS_AUX_PAYLOAD(work)) > than > if (work->aux_payload_sz) > > The unobfuscated version is actually shorter! Yep, looks better, Will fix it. Thanks for your review!
RE: [PATCH 3/5] cifsd: add file operations
>
> > -Original Message-
> > From: Namjae Jeon
> > Sent: Sunday, March 21, 2021 10:14 PM
> > To: linux-fsde...@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
> > c...@vger.kernel.org
> > Cc: linux-cifsd-de...@lists.sourceforge.net; smfre...@gmail.com;
> > senozhat...@chromium.org; hyc@gmail.com; v...@zeniv.linux.org.uk;
> > h...@lst.de; h...@infradead.org; ronniesahlb...@gmail.com;
> > aurelien.ap...@gmail.com; aap...@suse.com; sand...@sandeen.net;
> > dan.carpen...@oracle.com; colin.k...@canonical.com;
> > rdun...@infradead.org; Namjae Jeon ;
> > Sergey Senozhatsky ; Steve French
> >
> > Subject: [PATCH 3/5] cifsd: add file operations
> >
> > This adds file operations and buffer pool for cifsd.
> >
> > Signed-off-by: Namjae Jeon
> > Signed-off-by: Sergey Senozhatsky
> > Signed-off-by: Hyunchul Lee
> > Acked-by: Ronnie Sahlberg
> > Signed-off-by: Steve French
> > ---
> > fs/cifsd/buffer_pool.c | 292 ++
> > fs/cifsd/buffer_pool.h | 28 +
> > fs/cifsd/vfs.c | 1989
> > fs/cifsd/vfs.h | 314 +++
> > fs/cifsd/vfs_cache.c | 851 +
> > fs/cifsd/vfs_cache.h | 213 +
> > 6 files changed, 3687 insertions(+)
> > create mode 100644 fs/cifsd/buffer_pool.c
> > create mode 100644 fs/cifsd/buffer_pool.h
> > create mode 100644 fs/cifsd/vfs.c
> > create mode 100644 fs/cifsd/vfs.h
> > create mode 100644 fs/cifsd/vfs_cache.c
> > create mode 100644 fs/cifsd/vfs_cache.h
> >
> > diff --git a/fs/cifsd/buffer_pool.c b/fs/cifsd/buffer_pool.c
> > new file mode 100644
> > index ..864fea547c68
> > --- /dev/null
> > +++ b/fs/cifsd/buffer_pool.c
> > @@ -0,0 +1,292 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Copyright (C) 2018 Samsung Electronics Co., Ltd.
> > + */
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +#include "glob.h"
> > +#include "buffer_pool.h"
> > +#include "connection.h"
> > +#include "mgmt/ksmbd_ida.h"
> > +
> > +static struct kmem_cache *filp_cache;
> > +
> > +struct wm {
> > + struct list_headlist;
> > + unsigned intsz;
> > + charbuffer[0];
> > +};
> > +
> > +struct wm_list {
> > + struct list_headlist;
> > + unsigned intsz;
> > +
> > + spinlock_t wm_lock;
> > + int avail_wm;
> > + struct list_headidle_wm;
> > + wait_queue_head_t wm_wait;
> > +};
> > +
> > +static LIST_HEAD(wm_lists);
> > +static DEFINE_RWLOCK(wm_lists_lock);
> > +
> > +void *ksmbd_alloc(size_t size)
> > +{
> > + return kvmalloc(size, GFP_KERNEL | __GFP_ZERO);
> > +}
> > +
> > +void ksmbd_free(void *ptr)
> > +{
> > + kvfree(ptr);
> > +}
> > +
> > +static struct wm *wm_alloc(size_t sz, gfp_t flags)
> > +{
> > + struct wm *wm;
> > + size_t alloc_sz = sz + sizeof(struct wm);
> > +
> > + wm = kvmalloc(alloc_sz, flags);
> > + if (!wm)
> > + return NULL;
> > + wm->sz = sz;
> > + return wm;
> > +}
> > +
> > +static int register_wm_size_class(size_t sz)
> > +{
> > + struct wm_list *l, *nl;
> > +
> > + nl = kvmalloc(sizeof(struct wm_list), GFP_KERNEL);
> > + if (!nl)
> > + return -ENOMEM;
> > +
> > + nl->sz = sz;
> > + spin_lock_init(>wm_lock);
> > + INIT_LIST_HEAD(>idle_wm);
> > + INIT_LIST_HEAD(>list);
> > + init_waitqueue_head(>wm_wait);
> > + nl->avail_wm = 0;
> > +
> > + write_lock(_lists_lock);
> > + list_for_each_entry(l, _lists, list) {
> > + if (l->sz == sz) {
> > + write_unlock(_lists_lock);
> > + kvfree(nl);
> > + return 0;
> > + }
> > + }
> > +
> > + list_add(>list, _lists);
> > + write_unlock(_lists_lock);
> > + return 0;
> > +}
> > +
> > +static struct wm_list *match_wm_list(size_t size)
> > +{
> > + struct wm_list *l, *rl = NULL;
> > +
> > + read_lock(_lists_lock);
> > + list_for_each_entry(l, _lists, list) {
> > + if (l->sz == s
RE: [PATCH 3/5] cifsd: add file operations
> On Mon, Mar 22, 2021 at 02:13:42PM +0900, Namjae Jeon wrote:
> > This adds file operations and buffer pool for cifsd.
>
> Some random notes:
>
> > +static void rollback_path_modification(char *filename) {
> > + if (filename) {
> > + filename--;
> > + *filename = '/';
> What an odd way to spell filename[-1] = '/';...
Okay. Will fix.
>
> > +int ksmbd_vfs_inode_permission(struct dentry *dentry, int acc_mode,
> > +bool delete) {
>
> > + if (delete) {
> > + struct dentry *parent;
> > +
> > + parent = dget_parent(dentry);
> > + if (!parent)
> > + return -EINVAL;
> > +
> > + if (inode_permission(_user_ns, d_inode(parent), MAY_EXEC |
> > MAY_WRITE)) {
> > + dput(parent);
> > + return -EACCES;
> > + }
> > + dput(parent);
>
> Who's to guarantee that parent is stable? IOW, by the time of that
> inode_permission() call dentry might very well not be a child of that thing...
Okay, Will fix.
>
> > + parent = dget_parent(dentry);
> > + if (!parent)
> > + return 0;
> > +
> > + if (!inode_permission(_user_ns, d_inode(parent), MAY_EXEC |
> > MAY_WRITE))
> > + *daccess |= FILE_DELETE_LE;
>
> Ditto.
Okay.
>
> > +int ksmbd_vfs_mkdir(struct ksmbd_work *work,
> > + const char *name,
> > + umode_t mode)
>
>
> > + err = vfs_mkdir(_user_ns, d_inode(path.dentry), dentry, mode);
> > + if (!err) {
> > + ksmbd_vfs_inherit_owner(work, d_inode(path.dentry),
> > + d_inode(dentry));
>
> ->mkdir() might very well return success, with dentry left unhashed negative.
> Look at the callers of vfs_mkdir() to see how it should be handled.
Okay, Will fix.
>
> > +static int check_lock_range(struct file *filp,
> > + loff_t start,
> > + loff_t end,
> > + unsigned char type)
> > +{
> > + struct file_lock *flock;
> > + struct file_lock_context *ctx = file_inode(filp)->i_flctx;
> > + int error = 0;
> > +
> > + if (!ctx || list_empty_careful(>flc_posix))
> > + return 0;
> > +
> > + spin_lock(>flc_lock);
> > + list_for_each_entry(flock, >flc_posix, fl_list) {
> > + /* check conflict locks */
> > + if (flock->fl_end >= start && end >= flock->fl_start) {
> > + if (flock->fl_type == F_RDLCK) {
> > + if (type == WRITE) {
> > + ksmbd_err("not allow write by shared
> > lock\n");
> > + error = 1;
> > + goto out;
> > + }
> > + } else if (flock->fl_type == F_WRLCK) {
> > + /* check owner in lock */
> > + if (flock->fl_file != filp) {
> > + error = 1;
> > + ksmbd_err("not allow rw access by
> > exclusive lock from other
> opens\n");
> > + goto out;
> > + }
> > + }
> > + }
> > + }
> > +out:
> > + spin_unlock(>flc_lock);
> > + return error;
> > +}
>
> WTF is that doing in smbd?
This code was added to pass the smb2 lock test of samba's smbtorture.
Will fix it.
>
> > + filp = fp->filp;
> > + inode = d_inode(filp->f_path.dentry);
>
> That should be file_inode(). Try it on overlayfs, watch it do interesting
> things...
Okay.
>
> > + nbytes = kernel_read(filp, rbuf, count, pos);
> > + if (nbytes < 0) {
> > + name = d_path(>f_path, namebuf, sizeof(namebuf));
> > + if (IS_ERR(name))
> > + name = "(error)";
> > + ksmbd_err("smb read failed for (%s), err = %zd\n",
> > + name, nbytes);
>
> Do you really want the full pathname here? For (presumably) spew into syslog?
No, Will fix.
>
> > +int ksmbd_vfs_remove_file(struct ksmbd_work *work, char *name) {
> > + struct path parent;
> > + struct dentry *dir, *dentry;
> > + char *last;
> > + int err = -ENOENT;
> > +
> > + last = extract_last_component(name);
> > + if (!
RE: [PATCH 2/5] cifsd: add server-side procedures for SMB3
> On Mon, Mar 22, 2021 at 09:47:13AM +0300, Dan Carpenter wrote:
> > On Mon, Mar 22, 2021 at 02:13:41PM +0900, Namjae Jeon wrote:
> > > +static unsigned char
> > > +asn1_octet_decode(struct asn1_ctx *ctx, unsigned char *ch) {
> > > + if (ctx->pointer >= ctx->end) {
> > > + ctx->error = ASN1_ERR_DEC_EMPTY;
> > > + return 0;
> > > + }
> > > + *ch = *(ctx->pointer)++;
> > > + return 1;
> > > +}
> >
> >
> > Make this bool.
> >
>
> More importantly don't add another ANS1 parser, but use the generic one in
> lib/asn1_decoder.c instead.
> CIFS should also really use it.
Okay, Let me check it, cifs also...
Thanks!
RE: [PATCH 2/5] cifsd: add server-side procedures for SMB3
> On Mon, Mar 22, 2021 at 02:13:41PM +0900, Namjae Jeon wrote:
> > +static unsigned char
> > +asn1_octet_decode(struct asn1_ctx *ctx, unsigned char *ch) {
> > + if (ctx->pointer >= ctx->end) {
> > + ctx->error = ASN1_ERR_DEC_EMPTY;
> > + return 0;
> > + }
> > + *ch = *(ctx->pointer)++;
> > + return 1;
> > +}
>
>
> Make this bool.
Okay.
>
> > +
> > +static unsigned char
> > +asn1_tag_decode(struct asn1_ctx *ctx, unsigned int *tag) {
> > + unsigned char ch;
> > +
> > + *tag = 0;
> > +
> > + do {
> > + if (!asn1_octet_decode(ctx, ))
> > + return 0;
> > + *tag <<= 7;
> > + *tag |= ch & 0x7F;
> > + } while ((ch & 0x80) == 0x80);
> > + return 1;
> > +}
>
> Bool.
Okay.
>
> > +
> > +static unsigned char
> > +asn1_id_decode(struct asn1_ctx *ctx,
> > + unsigned int *cls, unsigned int *con, unsigned int *tag) {
> > + unsigned char ch;
> > +
> > + if (!asn1_octet_decode(ctx, ))
> > + return 0;
> > +
> > + *cls = (ch & 0xC0) >> 6;
> > + *con = (ch & 0x20) >> 5;
> > + *tag = (ch & 0x1F);
> > +
> > + if (*tag == 0x1F) {
> > + if (!asn1_tag_decode(ctx, tag))
> > + return 0;
> > + }
> > + return 1;
> > +}
>
>
> Same.
Okay.
>
> > +
> > +static unsigned char
> > +asn1_length_decode(struct asn1_ctx *ctx, unsigned int *def, unsigned
> > +int *len) {
> > + unsigned char ch, cnt;
> > +
> > + if (!asn1_octet_decode(ctx, ))
> > + return 0;
> > +
> > + if (ch == 0x80)
> > + *def = 0;
> > + else {
> > + *def = 1;
> > +
> > + if (ch < 0x80)
> > + *len = ch;
> > + else {
> > + cnt = (unsigned char) (ch & 0x7F);
> > + *len = 0;
> > +
> > + while (cnt > 0) {
> > + if (!asn1_octet_decode(ctx, ))
> > + return 0;
> > + *len <<= 8;
> > + *len |= ch;
> > + cnt--;
> > + }
> > + }
> > + }
> > +
> > + /* don't trust len bigger than ctx buffer */
> > + if (*len > ctx->end - ctx->pointer)
> > + return 0;
> > +
> > + return 1;
> > +}
>
>
> Same etc for all.
Okay.
>
> > +
> > +static unsigned char
> > +asn1_header_decode(struct asn1_ctx *ctx,
> > + unsigned char **eoc,
> > + unsigned int *cls, unsigned int *con, unsigned int *tag) {
> > + unsigned int def = 0;
> > + unsigned int len = 0;
> > +
> > + if (!asn1_id_decode(ctx, cls, con, tag))
> > + return 0;
> > +
> > + if (!asn1_length_decode(ctx, , ))
> > + return 0;
> > +
> > + /* primitive shall be definite, indefinite shall be constructed */
> > + if (*con == ASN1_PRI && !def)
> > + return 0;
> > +
> > + if (def)
> > + *eoc = ctx->pointer + len;
> > + else
> > + *eoc = NULL;
> > + return 1;
> > +}
> > +
> > +static unsigned char
> > +asn1_eoc_decode(struct asn1_ctx *ctx, unsigned char *eoc) {
> > + unsigned char ch;
> > +
> > + if (!eoc) {
> > + if (!asn1_octet_decode(ctx, ))
> > + return 0;
> > +
> > + if (ch != 0x00) {
> > + ctx->error = ASN1_ERR_DEC_EOC_MISMATCH;
> > + return 0;
> > + }
> > +
> > + if (!asn1_octet_decode(ctx, ))
> > + return 0;
> > +
> > + if (ch != 0x00) {
> > + ctx->error = ASN1_ERR_DEC_EOC_MISMATCH;
> > + return 0;
> > + }
> > + } else {
> > + if (ctx->pointer != eoc) {
> > + ctx->error = ASN1_ERR_DEC_LENGTH_MISMATCH;
> > + return 0;
> > + }
> > + }
> > + return 1;
> > +}
> > +
> > +static unsigned char
> > +asn1_subid_decode(struct asn1_ctx *ctx, unsigned long *subid) {
> > + unsigned char ch;
> > +
> > + *subid = 0;
&
[PATCH 5/5] MAINTAINERS: add cifsd kernel server
Add myself, Steve French, Sergey Senozhatsky and Hyunchul Lee as cifsd maintainer. Signed-off-by: Namjae Jeon Signed-off-by: Sergey Senozhatsky Signed-off-by: Hyunchul Lee Acked-by: Ronnie Sahlberg Signed-off-by: Steve French --- MAINTAINERS | 12 +++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/MAINTAINERS b/MAINTAINERS index aa84121c5611..30f678f8b4d3 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -4434,7 +4434,7 @@ F:include/linux/clk/ F: include/linux/of_clk.h X: drivers/clk/clkdev.c -COMMON INTERNET FILE SYSTEM (CIFS) +COMMON INTERNET FILE SYSTEM CLIENT (CIFS) M: Steve French L: linux-c...@vger.kernel.org L: samba-techni...@lists.samba.org (moderated for non-subscribers) @@ -,6 +,16 @@ T: git git://git.samba.org/sfrench/cifs-2.6.git F: Documentation/admin-guide/cifs/ F: fs/cifs/ +COMMON INTERNET FILE SYSTEM SERVER (CIFSD) +M: Namjae Jeon +M: Sergey Senozhatsky +M: Steve French +M: Hyunchul Lee +L: linux-c...@vger.kernel.org +L: linux-cifsd-de...@lists.sourceforge.net +S: Maintained +F: fs/cifsd/ + COMPACTPCI HOTPLUG CORE M: Scott Murray L: linux-...@vger.kernel.org -- 2.17.1
[PATCH 3/5] cifsd: add file operations
This adds file operations and buffer pool for cifsd.
Signed-off-by: Namjae Jeon
Signed-off-by: Sergey Senozhatsky
Signed-off-by: Hyunchul Lee
Acked-by: Ronnie Sahlberg
Signed-off-by: Steve French
---
fs/cifsd/buffer_pool.c | 292 ++
fs/cifsd/buffer_pool.h | 28 +
fs/cifsd/vfs.c | 1989
fs/cifsd/vfs.h | 314 +++
fs/cifsd/vfs_cache.c | 851 +
fs/cifsd/vfs_cache.h | 213 +
6 files changed, 3687 insertions(+)
create mode 100644 fs/cifsd/buffer_pool.c
create mode 100644 fs/cifsd/buffer_pool.h
create mode 100644 fs/cifsd/vfs.c
create mode 100644 fs/cifsd/vfs.h
create mode 100644 fs/cifsd/vfs_cache.c
create mode 100644 fs/cifsd/vfs_cache.h
diff --git a/fs/cifsd/buffer_pool.c b/fs/cifsd/buffer_pool.c
new file mode 100644
index ..864fea547c68
--- /dev/null
+++ b/fs/cifsd/buffer_pool.c
@@ -0,0 +1,292 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) 2018 Samsung Electronics Co., Ltd.
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include "glob.h"
+#include "buffer_pool.h"
+#include "connection.h"
+#include "mgmt/ksmbd_ida.h"
+
+static struct kmem_cache *filp_cache;
+
+struct wm {
+ struct list_headlist;
+ unsigned intsz;
+ charbuffer[0];
+};
+
+struct wm_list {
+ struct list_headlist;
+ unsigned intsz;
+
+ spinlock_t wm_lock;
+ int avail_wm;
+ struct list_headidle_wm;
+ wait_queue_head_t wm_wait;
+};
+
+static LIST_HEAD(wm_lists);
+static DEFINE_RWLOCK(wm_lists_lock);
+
+void *ksmbd_alloc(size_t size)
+{
+ return kvmalloc(size, GFP_KERNEL | __GFP_ZERO);
+}
+
+void ksmbd_free(void *ptr)
+{
+ kvfree(ptr);
+}
+
+static struct wm *wm_alloc(size_t sz, gfp_t flags)
+{
+ struct wm *wm;
+ size_t alloc_sz = sz + sizeof(struct wm);
+
+ wm = kvmalloc(alloc_sz, flags);
+ if (!wm)
+ return NULL;
+ wm->sz = sz;
+ return wm;
+}
+
+static int register_wm_size_class(size_t sz)
+{
+ struct wm_list *l, *nl;
+
+ nl = kvmalloc(sizeof(struct wm_list), GFP_KERNEL);
+ if (!nl)
+ return -ENOMEM;
+
+ nl->sz = sz;
+ spin_lock_init(>wm_lock);
+ INIT_LIST_HEAD(>idle_wm);
+ INIT_LIST_HEAD(>list);
+ init_waitqueue_head(>wm_wait);
+ nl->avail_wm = 0;
+
+ write_lock(_lists_lock);
+ list_for_each_entry(l, _lists, list) {
+ if (l->sz == sz) {
+ write_unlock(_lists_lock);
+ kvfree(nl);
+ return 0;
+ }
+ }
+
+ list_add(>list, _lists);
+ write_unlock(_lists_lock);
+ return 0;
+}
+
+static struct wm_list *match_wm_list(size_t size)
+{
+ struct wm_list *l, *rl = NULL;
+
+ read_lock(_lists_lock);
+ list_for_each_entry(l, _lists, list) {
+ if (l->sz == size) {
+ rl = l;
+ break;
+ }
+ }
+ read_unlock(_lists_lock);
+ return rl;
+}
+
+static struct wm *find_wm(size_t size)
+{
+ struct wm_list *wm_list;
+ struct wm *wm;
+
+ wm_list = match_wm_list(size);
+ if (!wm_list) {
+ if (register_wm_size_class(size))
+ return NULL;
+ wm_list = match_wm_list(size);
+ }
+
+ if (!wm_list)
+ return NULL;
+
+ while (1) {
+ spin_lock(_list->wm_lock);
+ if (!list_empty(_list->idle_wm)) {
+ wm = list_entry(wm_list->idle_wm.next,
+ struct wm,
+ list);
+ list_del(>list);
+ spin_unlock(_list->wm_lock);
+ return wm;
+ }
+
+ if (wm_list->avail_wm > num_online_cpus()) {
+ spin_unlock(_list->wm_lock);
+ wait_event(wm_list->wm_wait,
+ !list_empty(_list->idle_wm));
+ continue;
+ }
+
+ wm_list->avail_wm++;
+ spin_unlock(_list->wm_lock);
+
+ wm = wm_alloc(size, GFP_KERNEL);
+ if (!wm) {
+ spin_lock(_list->wm_lock);
+ wm_list->avail_wm--;
+ spin_unlock(_list->wm_lock);
+ wait_event(wm_list->wm_wait,
+ !list_empty(_list->idle_wm));
+ continue;
+ }
+ break;
+ }
+
+ return wm;
+}
+
+static void release_
[PATCH 4/5] cifsd: add Kconfig and Makefile
This adds the Kconfig and Makefile for cifsd. Signed-off-by: Namjae Jeon Signed-off-by: Sergey Senozhatsky Signed-off-by: Hyunchul Lee Acked-by: Ronnie Sahlberg Signed-off-by: Steve French --- fs/Kconfig| 1 + fs/Makefile | 1 + fs/cifsd/Kconfig | 64 +++ fs/cifsd/Makefile | 13 ++ 4 files changed, 79 insertions(+) create mode 100644 fs/cifsd/Kconfig create mode 100644 fs/cifsd/Makefile diff --git a/fs/Kconfig b/fs/Kconfig index a55bda4233bb..92deb66021d1 100644 --- a/fs/Kconfig +++ b/fs/Kconfig @@ -340,6 +340,7 @@ config NFS_V4_2_SSC_HELPER source "net/sunrpc/Kconfig" source "fs/ceph/Kconfig" source "fs/cifs/Kconfig" +source "fs/cifsd/Kconfig" source "fs/coda/Kconfig" source "fs/afs/Kconfig" source "fs/9p/Kconfig" diff --git a/fs/Makefile b/fs/Makefile index 3215fe205256..62dc87f3ff94 100644 --- a/fs/Makefile +++ b/fs/Makefile @@ -97,6 +97,7 @@ obj-$(CONFIG_NLS) += nls/ obj-$(CONFIG_UNICODE) += unicode/ obj-$(CONFIG_SYSV_FS) += sysv/ obj-$(CONFIG_CIFS) += cifs/ +obj-$(CONFIG_SMB_SERVER) += cifsd/ obj-$(CONFIG_HPFS_FS) += hpfs/ obj-$(CONFIG_NTFS_FS) += ntfs/ obj-$(CONFIG_UFS_FS) += ufs/ diff --git a/fs/cifsd/Kconfig b/fs/cifsd/Kconfig new file mode 100644 index ..6e78960be5b1 --- /dev/null +++ b/fs/cifsd/Kconfig @@ -0,0 +1,64 @@ +config SMB_SERVER + tristate "SMB server support (EXPERIMENTAL)" + depends on INET + select NLS + select NLS_UTF8 + select CRYPTO + select CRYPTO_MD4 + select CRYPTO_MD5 + select CRYPTO_HMAC + select CRYPTO_ARC4 + select CRYPTO_ECB + select CRYPTO_LIB_DES + select CRYPTO_SHA256 + select CRYPTO_CMAC + select CRYPTO_SHA512 + select CRYPTO_AEAD2 + select CRYPTO_CCM + select CRYPTO_GCM + default n + help + Choose Y here if you want to allow SMB3 compliant clients + to access files residing on this system using SMB3 protocol. + To compile the SMB3 server support as a module, + choose M here: the module will be called ksmbd. + + You may choose to use a samba server instead, in which + case you can choose N here. + + You also need to install user space programs which can be found + in cifsd-tools, available from + https://github.com/cifsd-team/cifsd-tools. + More detail about how to run the cifsd kernel server is + available via README file + (https://github.com/cifsd-team/cifsd-tools/blob/master/README). + + cifsd kernel server includes support for auto-negotiation, + Secure negotiate, Pre-authentication integrity, oplock/lease, + compound requests, multi-credit, packet signing, RDMA(smbdirect), + smb3 encryption, copy-offload, secure per-user session + establishment via NTLM or NTLMv2. + +config SMB_SERVER_SMBDIRECT + bool "Support for SMB Direct protocol" + depends on SMB_SERVER=m && INFINIBAND && INFINIBAND_ADDR_TRANS || SMB_SERVER=y && INFINIBAND=y && INFINIBAND_ADDR_TRANS=y + default n + + help + Enables SMB Direct support for SMB 3.0, 3.02 and 3.1.1. + + SMB Direct allows transferring SMB packets over RDMA. If unsure, + say N. + +config SMB_SERVER_CHECK_CAP_NET_ADMIN + bool "Enable check network administration capability" + depends on SMB_SERVER + default y + + help + Prevent unprivileged processes to start the cifsd kernel server. + +config SMB_SERVER_KERBEROS5 + bool "Support for Kerberos 5" + depends on SMB_SERVER + default n diff --git a/fs/cifsd/Makefile b/fs/cifsd/Makefile new file mode 100644 index ..a6c03c4ba51e --- /dev/null +++ b/fs/cifsd/Makefile @@ -0,0 +1,13 @@ +# SPDX-License-Identifier: GPL-2.0-or-later +# +# Makefile for Linux SMB3 kernel server +# +obj-$(CONFIG_SMB_SERVER) += ksmbd.o + +ksmbd-y := unicode.o auth.o vfs.o vfs_cache.o server.o buffer_pool.o \ + misc.o oplock.o connection.o ksmbd_work.o crypto_ctx.o \ + mgmt/ksmbd_ida.o mgmt/user_config.o mgmt/share_config.o \ + mgmt/tree_connect.o mgmt/user_session.o smb_common.o \ + transport_tcp.o transport_ipc.o smbacl.o smb2pdu.o \ + smb2ops.o smb2misc.o asn1.o netmisc.o ndr.o +ksmbd-$(CONFIG_SMB_SERVER_SMBDIRECT) += transport_rdma.o -- 2.17.1
[PATCH 0/5] cifsd: introduce new SMB3 kernel server
(auditing) is planned for the future. For ownership (SIDs) ksmbd generates random subauth values(then store it to disk) and use uid/gid get from inode as RID for local domain SID. The current acl implementation is limited to standalone server, not a domain member. Integration with Samba tools is being worked on to allow future support for running as a domain member. Kerberos Supported. Durable handle v1,v2 Planned for future. Persistent handle Planned for future. SMB2 notifyPlanned for future. Sparse file supportSupported. DCE/RPC supportPartially Supported. a few calls(NetShareEnumAll, NetServerGetInfo, SAMR, LSARPC) that are needed for file server handled via netlink interface from ksmbd.mountd. Additional integration with Samba tools and libraries via upcall is being investigated to allow support for additional DCE/RPC management calls (and future support for Witness protocol e.g.) == = All features required as file server are currently implemented in ksmbd. In particular, the implementation of SMB Direct(RDMA) is only currently possible with ksmbd (among Linux servers) Stability = It has been proved to be stable. A significant amount of xfstests pass and are run regularly from Linux to Linux: http://smb3-test-rhel-75.southcentralus.cloudapp.azure.com/#/builders/8/builds/26 In addition regression tests using the broadest SMB3 functional test suite (Samba's "smbtorture") are run on every checkin. It has already been used by many other open source toolkits and commercial companies that need NAS functionality. Their issues have been fixed and contributions are applied into ksmbd. Ksmbd has been well tested and verified in the field and market. Mailing list and repositories = - linux-cifsd-de...@lists.sourceforge.net - https://github.com/smfrench/smb3-kernel/tree/cifsd-for-next - https://github.com/cifsd-team/cifsd (out-of-tree) - https://github.com/cifsd-team/ksmbd-tools How to run ksmbd a. Download ksmbd-tools and compile them. - https://github.com/cifsd-team/ksmbd-tools b. Create user/password for SMB share. # mkdir /etc/ksmbd/ # ksmbd.adduser -a c. Create /etc/ksmbd/smb.conf file, add SMB share in smb.conf file - Refer smb.conf.example and Documentation/configuration.txt in ksmbd-tools d. Insert ksmbd.ko module # insmod ksmbd.ko e. Start ksmbd user space daemon # ksmbd.mountd f. Access share from Windows or Linux using SMB e.g. "mount -t cifs //server/share /mnt ..." v0: - fix a handful of spelling mistakes (Colin Ian King) - fix a precedence bug in parse_dacl() (Dan Carpenter) - fix a IS_ERR() vs NULL bug (Dan Carpenter) - fix a use after free on error path (Dan Carpenter) - update cifsd.rst Documentation - remove unneeded FIXME comments - fix static checker warnings (Dan Carpenter) - fix WARNING: unmet direct dependencies detected for CRYPTO_ARC4 (Randy Dunlap) - uniquify extract_sharename() (Stephen Rothwell) - fix WARNING: document isn't included in any toctree (Stephen Rothwell) - fix WARNING: Title overline too short (Stephen Rothwell) - fix incorrect function comments Namjae Jeon (5): cifsd: add server handler and tranport layers cifsd: add server-side procedures for SMB3 cifsd: add file operations cifsd: add Kconfig and Makefile MAINTAINERS: add cifsd kernel server Documentation/filesystems/cifs/cifsd.rst | 180 + Documentation/filesystems/cifs/index.rst | 10 + Documentation/filesystems/index.rst |2 +- MAINTAINERS | 12 +- fs/Kconfig |1 + fs/Makefile |1 + fs/cifsd/Kconfig | 64 + fs/cifsd/Makefile| 13 + fs/cifsd/asn1.c | 702 ++ fs/cifsd/asn1.h | 29 + fs/cifsd/auth.c | 1348 fs/cifsd/auth.h | 90 + fs/cifsd/buffer_pool.c | 292 + fs/cifsd/buffer_pool.h | 28 + fs/cifsd/connection.c| 416 ++ fs/cifsd/connection.h| 212 + fs/cifsd/crypto_ctx.c| 287 + fs/cifsd/crypto_ctx.h| 77 + fs/cifsd/glob.h
RE: [PATCH] exfat: improve write performance when dirsync enabled
> > Degradation of write speed caused by frequent disk access for cluster > > bitmap update on every cluster allocation could be improved by > > selective syncing bitmap buffer. Change to flush bitmap buffer only > > for the directory related operations. > > > > Signed-off-by: Hyeongseok Kim > > Looks good. > Thanks for your work. > > Acked-by: Sungjong Seo Applied. Thanks!
RE: linux-next: Tree for Mar 18 (cifsd: Kconfig)
> On 3/18/21 3:08 AM, Stephen Rothwell wrote:
> > Hi all,
> >
> > News: there will be no linux-next release on Friday this week.
> >
> > Warning: Some of the branches in linux-next are still based on
> > v5.12-rc1, so please be careful if you are trying to bisect a bug.
> >
> > News: if your -next included tree is based on Linus' tree tag
> > v5.12-rc1{,-dontuse} (or somewhere between v5.11 and that tag), please
> > consider rebasing it onto v5.12-rc2. Also, please check any branches
> > merged into your branch.
> >
> > Changes since 20210317:
> >
Hi Randy,
> > The cifsd tree lost its build failure.
>
> kconfig warning in cifsd:
>
> WARNING: unmet direct dependencies detected for CRYPTO_ARC4
> Depends on [n]: CRYPTO [=y] && CRYPTO_USER_API_ENABLE_OBSOLETE [=n]
> Selected by [y]:
> - SMB_SERVER [=y] && NETWORK_FILESYSTEMS [=y] && INET [=y]
>
>
>
> Either
> select CRYPTO_ARC4 if CRYPTO_USER_API_ENABLE_OBSOLETE or add
> select CRYPTO_USER_API
> select CRYPTO_USER_API_ENABLE_OBSOLETE
>
> The first choice is certainly more palatable if it is OK.
>
>
> thanks.
> --
> ~Randy
> Reported-by: Randy Dunlap
I will make the patch included your reported-by tag.
Thanks for your report and patch!
Re: [PATCH][next] cifsd: Fix a handful of spelling mistakes
2021-03-17 18:36 GMT+09:00, Colin King : > From: Colin Ian King > > There are several spelling mistakes in various ksmbd_err and > ksmbd_debug messages. Fix these. > > Signed-off-by: Colin Ian King Applied. Thanks for your patch!
RE: linux-next: build failure after merge of the cifsd tree
Hi Stephen,
> Hi all,
>
> After merging the cifsd tree, today's linux-next build (powerpc
> allyesconfig) failed like this:
>
> ld: fs/cifsd/misc.o:(.opd+0xc0): multiple definition of `extract_sharename';
> fs/cifs/unc.o:(.opd+0x18):
> first defined here
> ld: fs/cifsd/misc.o: in function `.extract_sharename':
> misc.c:(.text.extract_sharename+0x0): multiple definition of
> `.extract_sharename';
> fs/cifs/unc.o:unc.c:(.text.extract_sharename+0x0): first defined here
>
> Caused by commit
>
> cabcebc31de4 ("cifsd: introduce SMB3 kernel server")
>
> I applied the following patch for today:
I send a pull request included this patch to Steve.
Thanks for your patch.
>
> From: Stephen Rothwell
> Date: Wed, 17 Mar 2021 18:35:55 +1100
> Subject: [PATCH] cifsd: uniquify extract_sharename()
>
> Signed-off-by: Stephen Rothwell
> ---
> fs/cifsd/misc.c| 4 ++--
> fs/cifsd/misc.h| 2 +-
> fs/cifsd/smb2pdu.c | 2 +-
> fs/cifsd/unicode.h | 2 +-
> 4 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/fs/cifsd/misc.c b/fs/cifsd/misc.c index
> 9e689c33f7bb..2e83cfc43be9 100644
> --- a/fs/cifsd/misc.c
> +++ b/fs/cifsd/misc.c
> @@ -210,12 +210,12 @@ void ksmbd_conv_path_to_windows(char *path) }
>
> /**
> - * extract_sharename() - get share name from tree connect request
> + * cifsd_extract_sharename() - get share name from tree connect request
> * @treename:buffer containing tree name and share name
> *
> * Return: share name on success, otherwise error
> */
> -char *extract_sharename(char *treename)
> +char *cifsd_extract_sharename(char *treename)
> {
> char *name = treename;
> char *dst;
> diff --git a/fs/cifsd/misc.h b/fs/cifsd/misc.h index
> d67843aad509..4cb0d4bebb21 100644
> --- a/fs/cifsd/misc.h
> +++ b/fs/cifsd/misc.h
> @@ -25,7 +25,7 @@ void ksmbd_conv_path_to_unix(char *path); void
> ksmbd_strip_last_slash(char *path);
> void ksmbd_conv_path_to_windows(char *path);
>
> -char *extract_sharename(char *treename);
> +char *cifsd_extract_sharename(char *treename);
>
> char *convert_to_unix_name(struct ksmbd_share_config *share, char *name);
>
> diff --git a/fs/cifsd/smb2pdu.c b/fs/cifsd/smb2pdu.c index
> b20cc07ee809..3da96ebeae8b 100644
> --- a/fs/cifsd/smb2pdu.c
> +++ b/fs/cifsd/smb2pdu.c
> @@ -1709,7 +1709,7 @@ int smb2_tree_connect(struct ksmbd_work *work)
> goto out_err1;
> }
>
> - name = extract_sharename(treename);
> + name = cifsd_extract_sharename(treename);
> if (IS_ERR(name)) {
> status.ret = KSMBD_TREE_CONN_STATUS_ERROR;
> goto out_err1;
> diff --git a/fs/cifsd/unicode.h b/fs/cifsd/unicode.h index
> 228a02c9b95d..16269c098f86 100644
> --- a/fs/cifsd/unicode.h
> +++ b/fs/cifsd/unicode.h
> @@ -69,7 +69,7 @@ char *smb_strndup_from_utf16(const char *src, const int
> maxlen,
> const struct nls_table *codepage);
> extern int smbConvertToUTF16(__le16 *target, const char *source, int srclen,
> const struct nls_table *cp, int mapchars); -extern char
> *extract_sharename(char
> *treename);
> +extern char *cifsd_extract_sharename(char *treename);
> #endif
>
> wchar_t cifs_toupper(wchar_t in);
> --
> 2.30.0
>
> --
> Cheers,
> Stephen Rothwell
RE: linux-next: build warning after merge of the cifsd tree
Hi Stephen,
> Hi all,
>
> After merging the cifsd tree, today's linux-next build (htmldocs) produced
> this warning:
>
> Documentation/filesystems/cifs/cifsd.rst:3: WARNING: Title overline too short.
>
> =
> CIFSD - SMB3 Kernel Server
> =
> Documentation/filesystems/cifs/cifsd.rst: WARNING: document isn't included in
> any toctree
>
> Introduced by commit
>
> 597357deeecf ("cifsd: update cifsd.rst file")
I sent a pull request to Steve to fix this failure.
Thanks for your report!
>
> --
> Cheers,
> Stephen Rothwell
RE: [PATCH v4 0/2] Add FITRIM ioctl support for exFAT filesystem
> This is for adding FITRIM ioctl functionality to exFAT filesystem. > Firstly, because the fstrim is long operation, introduce bitmap_lock to > narrow the lock range to > prevent read operation stall. > After that, add generic ioctl function and FITRIM handler. > > Changelog > = > v3->v4: > - Introduce bitmap_lock mutex to narrow the lock range for bitmap access > and change to use bitmap_lock instead of s_lock in FITRIM handler to > prevent read stall while ongoing fstrim. > - Minor code style fix > > v2->v3: > - Remove unnecessary local variable > - Merge all changes to a single patch > > v1->v2: > - Change variable declaration order as reverse tree style. > - Return -EOPNOTSUPP from sb_issue_discard() just as it is. > - Remove cond_resched() in while loop. > - Move ioctl related code into it's helper function. > > Hyeongseok Kim (2): > exfat: introduce bitmap_lock for cluster bitmap access > exfat: add support ioctl and FITRIM function Applied. Thanks for your patches! > > fs/exfat/balloc.c | 80 + > fs/exfat/dir.c | 5 +++ > fs/exfat/exfat_fs.h | 5 +++ > fs/exfat/fatent.c | 37 - > fs/exfat/file.c | 53 ++ > fs/exfat/super.c| 1 + > 6 files changed, 173 insertions(+), 8 deletions(-) > > -- > 2.27.0.83.g0313f36
RE: [PATCH v2] exfat: fix erroneous discard when clear cluster bit
> If mounted with discard option, exFAT issues discard command when clear
> cluster bit to remove file.
> But the input parameter of cluster-to-sector calculation is abnormally added
> by reserved cluster size
> which is 2, leading to discard unrelated sectors included in target+2 cluster.
> With fixing this, remove the wrong comments in set/clear/find bitmap
> functions.
>
> Fixes: 1e49a94cf707 ("exfat: add bitmap operations")
Cc: sta...@vger.kernel.org # v5.7+
> Signed-off-by: Hyeongseok Kim
> Acked-by: Sungjong Seo
Applied. Thanks for your patch!
> ---
> fs/exfat/balloc.c | 15 +--
> 1 file changed, 1 insertion(+), 14 deletions(-)
>
> diff --git a/fs/exfat/balloc.c b/fs/exfat/balloc.c index
> 761c79c3a4ba..54f1bcbddb26 100644
> --- a/fs/exfat/balloc.c
> +++ b/fs/exfat/balloc.c
> @@ -141,10 +141,6 @@ void exfat_free_bitmap(struct exfat_sb_info *sbi)
> kfree(sbi->vol_amap);
> }
>
> -/*
> - * If the value of "clu" is 0, it means cluster 2 which is the first cluster
> of
> - * the cluster heap.
> - */
> int exfat_set_bitmap(struct inode *inode, unsigned int clu) {
> int i, b;
> @@ -162,10 +158,6 @@ int exfat_set_bitmap(struct inode *inode, unsigned int
> clu)
> return 0;
> }
>
> -/*
> - * If the value of "clu" is 0, it means cluster 2 which is the first cluster
> of
> - * the cluster heap.
> - */
> void exfat_clear_bitmap(struct inode *inode, unsigned int clu, bool sync) {
> int i, b;
> @@ -186,8 +178,7 @@ void exfat_clear_bitmap(struct inode *inode, unsigned int
> clu, bool sync)
> int ret_discard;
>
> ret_discard = sb_issue_discard(sb,
> - exfat_cluster_to_sector(sbi, clu +
> - EXFAT_RESERVED_CLUSTERS),
> + exfat_cluster_to_sector(sbi, clu),
> (1 << sbi->sect_per_clus_bits), GFP_NOFS, 0);
>
> if (ret_discard == -EOPNOTSUPP) {
> @@ -197,10 +188,6 @@ void exfat_clear_bitmap(struct inode *inode, unsigned
> int clu, bool sync)
> }
> }
>
> -/*
> - * If the value of "clu" is 0, it means cluster 2 which is the first cluster
> of
> - * the cluster heap.
> - */
> unsigned int exfat_find_free_bitmap(struct super_block *sb, unsigned int
> clu) {
> unsigned int i, map_i, map_b, ent_idx;
> --
> 2.27.0.83.g0313f36
[GIT PULL] exfat update for 5.12-rc1
Hi Linus, This is exfat update pull request for v5.12-rc1. I add description of this pull request on below. Please pull exfat with following ones. Thanks! The following changes since commit f40ddce88593482919761f74910f42f4b84c004b: Linux 5.11 (2021-02-14 14:32:24 -0800) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/linkinjeon/exfat.git tags/exfat-for-5.12-rc1 for you to fetch changes up to f728760aa923f1dd3a4818368dbdbd2c7d63b370: exfat: improve performance of exfat_free_cluster when using dirsync mount option (2021-02-22 09:55:14 +0900) Description for this pull request: - Improve file deletion performance with dirsync mount option. - fix shift-out-of-bounds in exfat_fill_super() generated by syzkaller. Hyeongseok Kim (1): exfat: improve performance of exfat_free_cluster when using dirsync mount option Namjae Jeon (1): exfat: fix shift-out-of-bounds in exfat_fill_super() fs/exfat/balloc.c| 4 ++-- fs/exfat/exfat_fs.h | 2 +- fs/exfat/exfat_raw.h | 4 fs/exfat/fatent.c| 43 +-- fs/exfat/super.c | 31 ++- 5 files changed, 70 insertions(+), 14 deletions(-)
Re: [PATCH v2 1/2] exfat: add initial ioctl function
2021-02-17 9:33 GMT+09:00, Hyeongseok Kim : > On 2/17/21 9:17 AM, Chaitanya Kulkarni wrote: >> On 2/16/21 16:13, Hyeongseok Kim wrote: >>> Sorry, I don't understand exactly. >>> You're saying that these 2 patch should be merged to a single patch? >>> Would it be better? >> I think so unless there is a specific reason for this to keep it >> isolated. >> > The reason was just that I think it seems better to seperate ioctl > initializing and adding specific ioctl functionality. > Anyway, I got it. > > Namjae, Hi Hyeongseok, > Do you have any other opinion about this? I also think this patch should be combined with the 2/2 patch. > If you agree, I'll merge these as one. Yep, Agreed. Please do that:) Thanks! > >
RE: [ANNOUNCE] exfatprogs-1.1.0 version released
> On Wed, Feb 10, 2021 at 12:50 AM Namjae Jeon wrote: > > > > Hi folk, > > > > We have released exfatprogs 1.1.0 version. In this release, exfatlabel > > has been added to print or re-write volume label and volume serial value. > > Also, A new dump.exfat util has been added to display statistics from > > a given device(Requested by Mike Fleetwood(GParted Developer)). > > > > Any feedback is welcome!:) > > > Hi Sedat, > Congrats to the new release and thanks to all involved people. Thanks! > > Hope Sven will do a new release for Debian. > ( Note that Debian/bullseye release plans "Milestone 2" this Friday, > February 12th (see [1] > "Key > release dates" > "[2021-Feb-12] Soft Freeze"). Dunno which impact this might > have on this. ) I hope he will do it, too! Thanks Sedat:) > > - Sedat - > > [1] https://release.debian.org/ > > > > CHANGES : > > * fsck.exfat: Recover corrupted boot region. > > > > NEW FEATURES : > > * exfatlabel: Print or set volume label and serial. > > * dump.exfat: Show the on-disk metadata information and the statistics. > > > > BUG FIXES : > > * Set _FILE_OFFSET_BITS=64 for Android build. > > > > The git tree is at: > > > > https://protect2.fireeye.com/v1/url?k=f588edef-aa13d460-f58966a0-0cc47 > > a31307c-ebe7fdcb9cce33c0=1=88dc7065-283e-4803-b82d-ffcf0f9d681e= > > https%3A%2F%2Fgithub.com%2Fexfatprogs%2Fexfatprogs > > > > The tarballs can be found at: > > > > https://protect2.fireeye.com/v1/url?k=98eca6ac-c7779f23-98ed2de3-0cc47a31307c- > c97058e3d3889dd3=1=88dc7065-283e-4803-b82d- > ffcf0f9d681e=https%3A%2F%2Fgithub.com%2Fexfatprogs%2Fexfatprogs%2Freleases%2Fdownload%2F1.1.0%2Fexfa > tprogs-1.1.0.tar.gz > >
[ANNOUNCE] exfatprogs-1.1.0 version released
Hi folk, We have released exfatprogs 1.1.0 version. In this release, exfatlabel has been added to print or re-write volume label and volume serial value. Also, A new dump.exfat util has been added to display statistics from a given device(Requested by Mike Fleetwood(GParted Developer)). Any feedback is welcome!:) CHANGES : * fsck.exfat: Recover corrupted boot region. NEW FEATURES : * exfatlabel: Print or set volume label and serial. * dump.exfat: Show the on-disk metadata information and the statistics. BUG FIXES : * Set _FILE_OFFSET_BITS=64 for Android build. The git tree is at: https://github.com/exfatprogs/exfatprogs The tarballs can be found at: https://github.com/exfatprogs/exfatprogs/releases/download/1.1.0/exfatprogs-1.1.0.tar.gz
RE: UBSAN: shift-out-of-bounds in exfat_fill_super
> On 1/25/21 10:39 AM, Matthew Wilcox wrote:
> > On Mon, Jan 25, 2021 at 09:33:14AM -0800, syzbot wrote:
> >> UBSAN: shift-out-of-bounds in fs/exfat/super.c:471:28 shift exponent
> >> 4294967294 is too large for 32-bit type 'int'
> >
> > This is an integer underflow:
> >
> > sbi->dentries_per_clu = 1 <<
> > (sbi->cluster_size_bits - DENTRY_SIZE_BITS);
> >
> > I think the problem is that there is no validation of sect_per_clus_bits.
> > We should check it is at least DENTRY_SIZE_BITS and probably that it's
> > less than ... 16? 64? I don't know what legitimate values are in
> > this field, but I would imagine that 255 is completely unacceptable.
>
> Ack all of that. The syzbot boot_sector has sect_per_clus_bits == 3 and
> sect_size_bits == 0, so sbi-
> >cluster_size_bits is 3, then UBSAN goes bang on:
>
> sbi->dentries_per_clu = 1 <<
> (sbi->cluster_size_bits - DENTRY_SIZE_BITS); // 3 - 5
>
>
> There is also an unprotected shift at line 480:
>
> if (sbi->num_FAT_sectors << p_boot->sect_size_bits <
> sbi->num_clusters * 4) {
>
> that should be protected IMO.
Right. I will also add validation for fat_length as well as sect_size_bits
before this.
Thanks!
>
>
> --
> ~Randy
RE: UBSAN: shift-out-of-bounds in exfat_fill_super
> > On Mon, Jan 25, 2021 at 09:33:14AM -0800, syzbot wrote: > > > UBSAN: shift-out-of-bounds in fs/exfat/super.c:471:28 shift exponent > > > 4294967294 is too large for 32-bit type 'int' > > > > This is an integer underflow: > > > > sbi->dentries_per_clu = 1 << > > (sbi->cluster_size_bits - DENTRY_SIZE_BITS); > > > > I think the problem is that there is no validation of sect_per_clus_bits. > > We should check it is at least DENTRY_SIZE_BITS and probably that it's > > less than ... 16? 64? I don't know what legitimate values are in this > > field, but I would imagine > that 255 is completely unacceptable. > exfat specification describe sect_per_clus_bits field of boot sector could be > at most 32 and at least typo ^^16 > 0. And sect_size_bits can also affect this calculation, It also needs > validation. > I will fix it. > Thanks!
RE: UBSAN: shift-out-of-bounds in exfat_fill_super
> On Mon, Jan 25, 2021 at 09:33:14AM -0800, syzbot wrote: > > UBSAN: shift-out-of-bounds in fs/exfat/super.c:471:28 shift exponent > > 4294967294 is too large for 32-bit type 'int' > > This is an integer underflow: > > sbi->dentries_per_clu = 1 << > (sbi->cluster_size_bits - DENTRY_SIZE_BITS); > > I think the problem is that there is no validation of sect_per_clus_bits. > We should check it is at least DENTRY_SIZE_BITS and probably that it's less > than ... 16? 64? I don't > know what legitimate values are in this field, but I would imagine that 255 > is completely unacceptable. exfat specification describe sect_per_clus_bits field of boot sector could be at most 32 and at least 0. And sect_size_bits can also affect this calculation, It also needs validation. I will fix it. Thanks!
RE: [PATCH] exfat: improve performance of exfat_free_cluster when using dirsync mount option
> > There are stressful update of cluster allocation bitmap when using > > dirsync mount option which is doing sync buffer on every cluster bit > > clearing. > > This could result in performance degradation when deleting big size file. > > Fix to update only when the bitmap buffer index is changed would make > > less disk access, improving performance especially for truncate operation. > > > > Testing with Samsung 256GB sdcard, mounted with dirsync option (mount > > -t exfat /dev/block/mmcblk0p1 /temp/mount -o dirsync) > > > > Remove 4GB file, blktrace result. > > [Before] : 39 secs. > > Total (blktrace): > > Reads Queued: 0,0KiB Writes Queued: 32775, > 16387KiB > > Read Dispatches: 0,0KiB Write Dispatches: 32775, > 16387KiB > > Reads Requeued:0Writes Requeued:0 > > Reads Completed: 0,0KiB Writes Completed: 32775, > 16387KiB > > Read Merges: 0,0KiB Write Merges: 0, > 0KiB > > IO unplugs:2Timer unplugs: 0 > > > > [After] : 1 sec. > > Total (blktrace): > > Reads Queued: 0,0KiB Writes Queued: 13, > 6KiB > > Read Dispatches: 0,0KiB Write Dispatches: 13, > 6KiB > > Reads Requeued:0Writes Requeued:0 > > Reads Completed: 0,0KiB Writes Completed: 13, > 6KiB > > Read Merges: 0,0KiB Write Merges: 0, > 0KiB > > IO unplugs:1Timer unplugs: 0 > > > > Signed-off-by: Hyeongseok Kim > > Looks good. > Thanks for your work! > > Acked-by: Sungjong Seo Applied. Thanks!
[GIT PULL] exfat update for 5.11-rc1
Hi Linus, This is exfat update pull request for v5.11-rc1. I add description of this pull request on below. Please pull exfat with following ones. Thanks! The following changes since commit 2c85ebc57b3e1817b6ce1a6b703928e113a90442: Linux 5.10 (2020-12-13 14:41:30 -0800) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/linkinjeon/exfat.git tags/exfat-for-5.11-rc1 for you to fetch changes up to 9eb78c25327548b905598975aa3ded4ef244b94a: exfat: Avoid allocating upcase table using kcalloc() (2020-12-22 12:31:17 +0900) Description for this pull request: - Avoid page allocation failure from upcase table allocation. Artem Labazov (1): exfat: Avoid allocating upcase table using kcalloc() fs/exfat/nls.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
RE: [PATCH v2] exfat: Avoid allocating upcase table using kcalloc()
> The table for Unicode upcase conversion requires an order-5 allocation, which > may fail on a highly- > fragmented system: > > pool-udisksd: page allocation failure: order:5, > mode:0x40dc0(GFP_KERNEL|__GFP_COMP|__GFP_ZERO), > nodemask=(null),cpuset=/,mems_allowed=0 > CPU: 4 PID: 3756880 Comm: pool-udisksd Tainted: G U > 5.8.10-200.fc32.x86_64 #1 > Hardware name: Dell Inc. XPS 13 9360/0PVG6D, BIOS 2.13.0 11/14/2019 Call > Trace: > dump_stack+0x6b/0x88 > warn_alloc.cold+0x75/0xd9 > ? _cond_resched+0x16/0x40 > ? __alloc_pages_direct_compact+0x144/0x150 > __alloc_pages_slowpath.constprop.0+0xcfa/0xd30 > ? __schedule+0x28a/0x840 > ? __wait_on_bit_lock+0x92/0xa0 > __alloc_pages_nodemask+0x2df/0x320 > kmalloc_order+0x1b/0x80 > kmalloc_order_trace+0x1d/0xa0 > exfat_create_upcase_table+0x115/0x390 [exfat] > exfat_fill_super+0x3ef/0x7f0 [exfat] > ? sget_fc+0x1d0/0x240 > ? exfat_init_fs_context+0x120/0x120 [exfat] > get_tree_bdev+0x15c/0x250 > vfs_get_tree+0x25/0xb0 > do_mount+0x7c3/0xaf0 > ? copy_mount_options+0xab/0x180 > __x64_sys_mount+0x8e/0xd0 > do_syscall_64+0x4d/0x90 > entry_SYSCALL_64_after_hwframe+0x44/0xa9 > > Make the driver use vzalloc() to eliminate the issue. > > Cc: sta...@vger.kernel.org # v5.7+ > Signed-off-by: Artem Labazov <123321art...@gmail.com> > --- > v2: replace vmalloc with vzalloc to avoid uninitialized memory access Applied. Thanks for your work!
[GIT PULL] exfat update for 5.10-rc1
Hi Linus, This is exfat update pull request for v5.10-rc1. I add description of this pull request on below. Please pull exfat with following ones. Thanks! The following changes since commit bbf5c979011a099af5dc76498918ed7df445635b: Linux 5.9 (2020-10-11 14:15:50 -0700) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/linkinjeon/exfat.git tags/exfat-for-5.10-rc1 for you to fetch changes up to eae503f7eb0509594076a951e422e29082385c96: exfat: remove useless check in exfat_move_file() (2020-10-22 08:29:12 +0900) Description for this pull request: - Replace memcpy with structure assignment. - Remove unneeded codes and use helper function i_blocksize(). - Fix typos found by codespell. Namjae Jeon (1): exfat: fix misspellings using codespell tool Tetsuhiro Kohada (5): exfat: eliminate dead code in exfat_find() exfat: remove useless directory scan in exfat_add_entry() exfat: replace memcpy with structure assignment exfat: remove 'rwoffset' in exfat_inode_info exfat: remove useless check in exfat_move_file() Xianting Tian (1): exfat: use i_blocksize() to get blocksize fs/exfat/dir.c | 29 -- fs/exfat/exfat_fs.h | 4 +- fs/exfat/file.c | 4 +- fs/exfat/inode.c| 5 +- fs/exfat/namei.c| 153 +++- fs/exfat/nls.c | 2 +- fs/exfat/super.c| 1 - 7 files changed, 71 insertions(+), 127 deletions(-)
RE: [PATCH v3 2/2] exfat: aggregate dir-entry updates into __exfat_write_inode().
> *inode) static int exfat_map_cluster(struct inode *inode, unsigned int
> clu_offset,
> unsigned int *clu, int create)
> {
> - int ret, modified = false;
> + int ret;
> unsigned int last_clu;
> struct exfat_chain new_clu;
> struct super_block *sb = inode->i_sb;
> @@ -184,6 +185,11 @@ static int exfat_map_cluster(struct inode *inode,
> unsigned int clu_offset,
> return -EIO;
> }
>
> + exfat_warn(sb, "alloc[%lu]@map: %lld (%d - %08x)",
> +inode->i_ino, i_size_read(inode),
> +(clu_offset << sbi->sect_per_clus_bits) * 512,
> +last_clu);
Is this leftover print from debugging?
[GIT PULL] exfat fixes for 5.9-rc9
Hi Linus, This is exfat fixes pull request for v5.9-rc9. I add description of this pull request on below. Please pull exfat with following fixes. Thanks! The following changes since commit 549738f15da0e5a00275977623be199fbbf7df50: Linux 5.9-rc8 (2020-10-04 16:04:34 -0700) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/linkinjeon/exfat.git tags/exfat-for-5.9-rc9 for you to fetch changes up to 8ff006e57ad3a25f909c456d053aa498b6673a39: exfat: fix use of uninitialized spinlock on error path (2020-10-07 14:27:13 +0900) Description for this pull request: - Fix use of uninitialized spinlock on error path. - Fix missing err assignment in exfat_build_inode(). Namjae Jeon (1): exfat: fix use of uninitialized spinlock on error path Tetsuhiro Kohada (1): exfat: fix pointer error checking fs/exfat/cache.c| 11 --- fs/exfat/exfat_fs.h | 3 ++- fs/exfat/inode.c| 2 -- fs/exfat/namei.c| 13 ++--- fs/exfat/super.c| 5 - 5 files changed, 12 insertions(+), 22 deletions(-)
RE: [PATCH v3 1/2] exfat: add exfat_update_inode()
> @@ -1352,19 +1340,13 @@ static int exfat_rename(struct inode *old_dir, struct > dentry *old_dentry, > new_dir->i_ctime = new_dir->i_mtime = new_dir->i_atime = > EXFAT_I(new_dir)->i_crtime = current_time(new_dir); > exfat_truncate_atime(_dir->i_atime); > - if (IS_DIRSYNC(new_dir)) > - exfat_sync_inode(new_dir); > - else > - mark_inode_dirty(new_dir); > + exfat_update_inode(new_dir); > > i_pos = ((loff_t)EXFAT_I(old_inode)->dir.dir << 32) | > (EXFAT_I(old_inode)->entry & 0x); > exfat_unhash_inode(old_inode); > exfat_hash_inode(old_inode, i_pos); > - if (IS_DIRSYNC(new_dir)) > - exfat_sync_inode(old_inode); > - else > - mark_inode_dirty(old_inode); > + exfat_update_inode(old_inode); This is checking if old_inode is IS_DIRSYNC, not new_dir. Is there any reason ?
RE: [PATCH 2/3] exfat: remove useless check in exfat_move_file()
> > >> --- a/fs/exfat/namei.c > > >> +++ b/fs/exfat/namei.c > > >> @@ -1095,11 +1095,6 @@ static int exfat_move_file(struct inode > > >> *inode, struct exfat_chain *p_olddir, > > >> if (!epmov) > > >> return -EIO; > > >> > > >> -/* check if the source and target directory is the same */ > > >> -if (exfat_get_entry_type(epmov) == TYPE_DIR && > > >> -le32_to_cpu(epmov->dentry.stream.start_clu) == > > >> p_newdir->dir) > > >> -return -EINVAL; > > >> - > > > > > > It might check if the cluster numbers are same between source entry > > > and target directory. > > > > This checks if newdir is the move target itself. > > Example: > >mv /mnt/dir0 /mnt/dir0/foo > > > > However, this check is not enough. > > We need to check newdir and all ancestors. > > Example: > >mv /mnt/dir0 /mnt/dir0/dir1/foo > >mv /mnt/dir0 /mnt/dir0/dir1/dir2/foo > >... > > > > This is probably a taboo for all layered filesystems. > > > > > > > Could you let me know what code you mentioned? > > > Or do you mean the codes on vfs? > > > > You can find in do_renameat2(). --- around 'fs/namei.c:4440' > > If the destination ancestors are itself, our driver will not be called. > > I think, of course, vfs has been doing that. > So that code is unnecessary in normal situations. > > That code comes from the old exfat implementation. > And as far as I understand, it seems to check once more "the cluster number" > even though it comes through vfs so that it tries detecting abnormal of > on-disk. > > Anyway, I agonized if it is really needed. > In conclusion, old code could be eliminated and your patch looks reasonable. > Thanks > > Acked-by: Sungjong Seo > > > > > > > BTW > > Are you busy now? > I'm sorry, I'm so busy for my full time work :( Anyway, I'm trying to review > serious bug patches or > bug reports first. > Other patches, such as clean-up or code refactoring, may take some time to > review. > > > I am waiting for your reply about "integrates dir-entry getting and > > validation" patch. > As I know, your patch is being under review by Namjae. I already gave comments and a patch, but you said you can't do it. I'm sorry, But I can't accept an incomplete patch. I will directly fix it later. > > > > > BR > > --- > > Tetsuhiro Kohada
RE: [PATCH v2] exfat: remove 'rwoffset' in exfat_inode_info
> > Remove 'rwoffset' in exfat_inode_info and replace it with the > > parameter of exfat_readdir(). > > Since rwoffset is referenced only by exfat_readdir(), it is not > > necessary a exfat_inode_info's member. > > Also, change cpos to point to the next of entry-set, and return the > > index of dir-entry via dir_entry->entry. > > > > Signed-off-by: Tetsuhiro Kohada > > Acked-by: Sungjong Seo Applied, Thanks for your patch!
Re: [PATCH 3/3] exfat: replace memcpy with structure assignment
2020-09-15 19:23 GMT-07:00, Sungjong Seo : >> Use structure assignment instead of memcpy. >> >> Signed-off-by: Tetsuhiro Kohada > > Acked-by: Sungjong Seo Applied. Thanks for your patch!
Re: [PATCH 1/3] exfat: remove useless directory scan in exfat_add_entry()
2020-09-15 19:22 GMT-07:00, Sungjong Seo : >> There is nothing in directory just created, so there is no need to scan. >> >> Signed-off-by: Tetsuhiro Kohada > > Acked-by: Sungjong Seo Applied. Thanks for your patch!
RE: [PATCH] exfat: eliminate dead code in exfat_find()
> > The exfat_find_dir_entry() called by exfat_find() doesn't return -EEXIST. > > Therefore, the root-dir information setting is never executed. > > > > Signed-off-by: Tetsuhiro Kohada > > Acked-by: Sungjong Seo Applied. Thanks for your work!
RE: [PATCH v4 1/5] exfat: integrates dir-entry getting and validation
> + i = ES_INDEX_NAME;
> + while ((ep = exfat_get_validated_dentry(es, i++, TYPE_NAME))) {
Please find the way to access name entries like ep_file, ep_stream
without calling exfat_get_validated_dentry().
> exfat_extract_uni_name(ep, uniname);
> uniname += EXFAT_FILE_NAME_LEN;
> }
> @@ -372,7 +369,7 @@ unsigned int exfat_get_entry_type(struct exfat_dentry *ep)
> if (ep->type == EXFAT_STREAM)
> return TYPE_STREAM;
> if (ep->type == EXFAT_NAME)
> - return TYPE_EXTEND;
> + return TYPE_NAME;
> if (ep->type == EXFAT_ACL)
> return TYPE_ACL;
> return TYPE_CRITICAL_SEC;
> @@ -388,7 +385,7 @@ static void exfat_set_entry_type(struct exfat_dentry *ep,
> unsigned int type)
> ep->type &= EXFAT_DELETE;
> } else if (type == TYPE_STREAM) {
> ep->type = EXFAT_STREAM;
> - } else if (type == TYPE_EXTEND) {
> + } else if (type == TYPE_NAME) {
> ep->type = EXFAT_NAME;
> } else if (type == TYPE_BITMAP) {
> ep->type = EXFAT_BITMAP;
> @@ -421,7 +418,7 @@ static void exfat_init_name_entry(struct exfat_dentry
> *ep, {
> int i;
>
> - exfat_set_entry_type(ep, TYPE_EXTEND);
> + exfat_set_entry_type(ep, TYPE_NAME);
> ep->dentry.name.flags = 0x0;
>
> for (i = 0; i < EXFAT_FILE_NAME_LEN; i++) { @@ -550,7 +547,7 @@ int
> exfat_init_ext_entry(struct
> inode *inode, struct exfat_chain *p_dir,
> exfat_update_bh(bh, sync);
> brelse(bh);
>
> - for (i = EXFAT_FIRST_CLUSTER; i < num_entries; i++) {
> + for (i = ES_INDEX_NAME; i < num_entries; i++) {
> ep = exfat_get_dentry(sb, p_dir, entry + i, , );
> if (!ep)
> return -EIO;
> @@ -590,17 +587,16 @@ int exfat_remove_entries(struct inode *inode, struct
> exfat_chain *p_dir, void
> exfat_update_dir_chksum_with_entry_set(struct exfat_entry_set_cache *es) {
> int chksum_type = CS_DIR_ENTRY, i;
> - unsigned short chksum = 0;
> + u16 chksum = 0;
> struct exfat_dentry *ep;
>
> for (i = 0; i < es->num_entries; i++) {
> - ep = exfat_get_dentry_cached(es, i);
> + ep = exfat_get_validated_dentry(es, i, TYPE_ALL);
Ditto, You do not need to repeatedly call exfat_get_validated_dentry() for the
entries
which got from exfat_get_dentry_set().
> chksum = exfat_calc_chksum16(ep, DENTRY_SIZE, chksum,
>chksum_type);
> chksum_type = CS_DEFAULT;
> }
> - ep = exfat_get_dentry_cached(es, 0);
> - ep->dentry.file.checksum = cpu_to_le16(chksum);
> + ES_FILE(es).checksum = cpu_to_le16(chksum);
> es->modified = true;
> }
>
> struct exfat_entry_set_cache *exfat_get_dentry_set(struct super_block *sb,
> - struct exfat_chain *p_dir, int entry, unsigned int type)
> + struct exfat_chain *p_dir, int entry, int max_entries)
> {
> int ret, i, num_bh;
> - unsigned int off, byte_offset, clu = 0;
> + unsigned int byte_offset, clu = 0;
> sector_t sec;
> struct exfat_sb_info *sbi = EXFAT_SB(sb);
> struct exfat_entry_set_cache *es;
> - struct exfat_dentry *ep;
> - int num_entries;
> - enum exfat_validate_dentry_mode mode = ES_MODE_STARTED;
> struct buffer_head *bh;
>
> if (p_dir->dir == DIR_DELETED) {
> @@ -844,13 +811,13 @@ struct exfat_entry_set_cache
> *exfat_get_dentry_set(struct super_block *sb,
> return NULL;
> es->sb = sb;
> es->modified = false;
> + es->num_entries = 1;
>
> /* byte offset in cluster */
> byte_offset = EXFAT_CLU_OFFSET(byte_offset, sbi);
>
> /* byte offset in sector */
> - off = EXFAT_BLK_OFFSET(byte_offset, sb);
> - es->start_off = off;
> + es->start_off = EXFAT_BLK_OFFSET(byte_offset, sb);
>
> /* sector offset in cluster */
> sec = EXFAT_B_TO_BLK(byte_offset, sb); @@ -861,15 +828,12 @@ struct
> exfat_entry_set_cache
> *exfat_get_dentry_set(struct super_block *sb,
> goto free_es;
> es->bh[es->num_bh++] = bh;
>
> - ep = exfat_get_dentry_cached(es, 0);
> - if (!exfat_validate_entry(exfat_get_entry_type(ep), ))
> + es->ep_file = exfat_get_validated_dentry(es, ES_INDEX_FILE, TYPE_FILE);
> + if (!es->ep_file)
> goto free_es;
> + es->num_entries = min(ES_FILE(es).num_ext + 1, max_entries);
>
> - num_entries = type == ES_ALL_ENTRIES ?
> - ep->dentry.file.num_ext + 1 : type;
> - es->num_entries = num_entries;
> -
> - num_bh = EXFAT_B_TO_BLK_ROUND_UP(off + num_entries * DENTRY_SIZE, sb);
> + num_bh = EXFAT_B_TO_BLK_ROUND_UP(es->start_off + es->num_entries *
> +DENTRY_SIZE, sb);
> for (i = 1; i < num_bh; i++) {
> /* get the next sector */
> if
RE: [PATCH] exfat: fix pointer error checking
> Fix missing result check of exfat_build_inode(). > And use PTR_ERR_OR_ZERO instead of PTR_ERR. > > Signed-off-by: Tetsuhiro Kohada Pushed it to exfat #dev. Thanks for your patch!
RE: [PATCH v3] exfat: integrates dir-entry getting and validation
> Thank you for quick reply!
>
> On 2020/08/26 13:19, Namjae Jeon wrote:
> >> On 2020/08/26 10:03, Namjae Jeon wrote:
> >>>> Second: Range validation and type validation should not be separated.
> >>>> When I started making this patch, I intended to add only range
> >>>> validation.
> >>>> However, after the caller gets the ep, the type validation follows.
> >>>> Get ep, null check of ep (= range verification), type verification is a
> >>>> series of procedures.
> >>>> There would be no reason to keep them independent anymore.
> >>>> Range and type validation is enforced when the caller uses ep.
> >>> You can add a validate flags as argument of exfat_get_dentry_set(), e.g.
> >>> none, basic and strict.
> >>> none : only range validation.
> >>> basic : range + type validation.
> >>> strict : range + type + checksum and name length, etc.
> >>
> >> Currently, various types of verification will not be needed.
> >> Let's add it when we need it.
> >>>
> >>>>> - /* validiate cached dentries */
> >>>>> - for (i = 1; i < num_entries; i++) {
> >>>>> - ep = exfat_get_dentry_cached(es, i);
> >>>>> - if (!exfat_validate_entry(exfat_get_entry_type(ep),
> >>>>> ))
> >>>>> + ep = exfat_get_dentry_cached(es, ENTRY_STREAM);
> >>>>> + if (!ep || ep->type != EXFAT_STREAM)
> >>>>> + goto free_es;
> >>>>> + es->de[ENTRY_STREAM] = ep;
> >>>>
> >>>> The value contained in stream-ext dir-entry should not be used
> >>>> before validating the EntrySet
> >> checksum.
> >>>> So I would insert EntrySet checksum validation here.
> >>>> In that case, the checksum verification loop would be followed by
> >>>> the TYPE_NAME verification loop, can you acceptable?
> >>> Yes. That would be great.
> >>
> >> OK.
> >> I'll add TYPE_NAME verification after checksum verification, in next patch.
> >> However, I think it is enough to validate TYPE_NAME when extracting name.
> >> Could you please tell me why you think you need TYPE_NAME validation here?
> > I've told you on previous mail. This function should return validated
> > dentry set after checking
> > file->stream->name in sequence.
>
> Yes. I understand that the current implementation checks in that order.
> Sorry, my question was unclear.
> Why do you think you should leave the TYPE_NAME validation in this function?
> What kind of problem are you worried about if this function does not validate
> TYPE_NAME?
> (for preserve the current behavior?)
We have not checked the problem when it is removed because it was implemented
according to the specification from the beginning. And your v3 patch are
already checking the name entries as TYPE_SECONDARY. And it check them with
TYPE_NAME again in exfat_get_uniname_from_ext_entry(). If you check TYPE_NAME
with stream->name_len, We don't need to perform the loop for extracting
filename from the name entries if stream->name_len or name entry is invalid.
And I request to prove why we do not need to validate name entries in this
function calling from somewhere. So as I suggested earlier, You can make it
with an argument flags so that we skip the validation.
>
> Don't worry, I will add TYPE_NAME verification to the v4 patch.
> I will post it later today.
Sound good.
>
> BR
> ---
> Tetsuhiro Kohada
RE: [PATCH v3] exfat: integrates dir-entry getting and validation
> On 2020/08/26 10:03, Namjae Jeon wrote:
> >> Second: Range validation and type validation should not be separated.
> >> When I started making this patch, I intended to add only range validation.
> >> However, after the caller gets the ep, the type validation follows.
> >> Get ep, null check of ep (= range verification), type verification is a
> >> series of procedures.
> >> There would be no reason to keep them independent anymore.
> >> Range and type validation is enforced when the caller uses ep.
> > You can add a validate flags as argument of exfat_get_dentry_set(), e.g.
> > none, basic and strict.
> > none : only range validation.
> > basic : range + type validation.
> > strict : range + type + checksum and name length, etc.
>
> Currently, various types of verification will not be needed.
> Let's add it when we need it.
> >
> >>> - /* validiate cached dentries */
> >>> - for (i = 1; i < num_entries; i++) {
> >>> - ep = exfat_get_dentry_cached(es, i);
> >>> - if (!exfat_validate_entry(exfat_get_entry_type(ep), ))
> >>> + ep = exfat_get_dentry_cached(es, ENTRY_STREAM);
> >>> + if (!ep || ep->type != EXFAT_STREAM)
> >>> + goto free_es;
> >>> + es->de[ENTRY_STREAM] = ep;
> >>
> >> The value contained in stream-ext dir-entry should not be used before
> >> validating the EntrySet
> checksum.
> >> So I would insert EntrySet checksum validation here.
> >> In that case, the checksum verification loop would be followed by the
> >> TYPE_NAME verification loop, can you acceptable?
> > Yes. That would be great.
>
> OK.
> I'll add TYPE_NAME verification after checksum verification, in next patch.
> However, I think it is enough to validate TYPE_NAME when extracting name.
> Could you please tell me why you think you need TYPE_NAME validation here?
I've told you on previous mail. This function should return validated dentry
set after checking
file->stream->name in sequence.
>
>
> BR
> ---
> Tetsuhiro Kohada
> >
RE: [PATCH v3] exfat: integrates dir-entry getting and validation
> Second: Range validation and type validation should not be separated.
> When I started making this patch, I intended to add only range validation.
> However, after the caller gets the ep, the type validation follows.
> Get ep, null check of ep (= range verification), type verification is a
> series of procedures.
> There would be no reason to keep them independent anymore.
> Range and type validation is enforced when the caller uses ep.
You can add a validate flags as argument of exfat_get_dentry_set(), e.g. none,
basic and strict.
none : only range validation.
basic : range + type validation.
strict : range + type + checksum and name length, etc.
> > - /* validiate cached dentries */
> > - for (i = 1; i < num_entries; i++) {
> > - ep = exfat_get_dentry_cached(es, i);
> > - if (!exfat_validate_entry(exfat_get_entry_type(ep), ))
> > + ep = exfat_get_dentry_cached(es, ENTRY_STREAM);
> > + if (!ep || ep->type != EXFAT_STREAM)
> > + goto free_es;
> > + es->de[ENTRY_STREAM] = ep;
>
> The value contained in stream-ext dir-entry should not be used before
> validating the EntrySet checksum.
> So I would insert EntrySet checksum validation here.
> In that case, the checksum verification loop would be followed by the
> TYPE_NAME verification loop, can
> you acceptable?
Yes. That would be great.
Thanks!
>
>
> > diff --git a/fs/exfat/exfat_fs.h b/fs/exfat/exfat_fs.h index
> > 44dc04520175..0e4cc8ba2f8e 100644
> > --- a/fs/exfat/exfat_fs.h
> > +++ b/fs/exfat/exfat_fs.h
> > @@ -33,6 +33,12 @@ enum {
> > NLS_NAME_OVERLEN, /* the length is over than its limit */
> > };
> >
> > +enum {
> > + ENTRY_FILE,
> > + ENTRY_STREAM,
> > + ENTRY_NAME,
> > +};
>
> This is necessary!
> With this, some magic numbers will be gone.
> But, I think it's better to use a name that can be recognized as an
> offset/index in the EntrySet.
> And, I think it's better to define this in "exfat_raw.h"
Okay, You can rename it and move it to there.
RE: [PATCH v3] exfat: integrates dir-entry getting and validation
> Thank you for your reply.
>
> >> @@ -171,7 +174,9 @@ struct exfat_entry_set_cache {
> >>unsigned int start_off;
> >>int num_bh;
> >>struct buffer_head *bh[DIR_CACHE_SIZE];
> >> - unsigned int num_entries;
> >> + int num_entries;
> >> + struct exfat_de_file *de_file;
> >> + struct exfat_de_stream *de_stream;
> > I prefer to assign validated entries to **de and use it using enum value.
> > struct exfat_dentry **de;
>
> I've tried several implementations that add a struct exfat_dentry type.(*de0
> & *de1; *de[2]; etc...)
> The problem with the struct exfat_dentry type is that it is too flexible for
> type.
> This means weak typing.
> Therefore, when using them,
> de[XXX_FILE]->dentry.file.zzz ...
> It is necessary to re-specify the type. (against the DRY principle) Strong
> typing prevents use with
> wrong type, at compiling.
>
> I think the approach of using de_file/de_stream could be strongly typed.
> I don't think we need excessive flexibility.
Could you check the following change ?
If you think it's okay, please add it to your patch series.
diff --git a/fs/exfat/dir.c b/fs/exfat/dir.c
index 573659bfbc55..37e0f92f74b3 100644
--- a/fs/exfat/dir.c
+++ b/fs/exfat/dir.c
@@ -44,14 +44,12 @@ static void exfat_get_uniname_from_ext_entry(struct
super_block *sb,
* Third entry : first file-name entry
* So, the index of first file-name dentry should start from 2.
*/
- for (i = 2; i < es->num_entries; i++) {
- struct exfat_dentry *ep = exfat_get_dentry_cached(es, i);
-
+ for (i = ENTRY_NAME; i < es->num_entries; i++) {
/* end of name entry */
- if (exfat_get_entry_type(ep) != TYPE_EXTEND)
+ if (exfat_get_entry_type(es->de[i]) != TYPE_NAME)
break;
- exfat_extract_uni_name(ep, uniname);
+ exfat_extract_uni_name(es->de[i], uniname);
uniname += EXFAT_FILE_NAME_LEN;
}
@@ -372,7 +370,7 @@ unsigned int exfat_get_entry_type(struct exfat_dentry *ep)
if (ep->type == EXFAT_STREAM)
return TYPE_STREAM;
if (ep->type == EXFAT_NAME)
- return TYPE_EXTEND;
+ return TYPE_NAME;
if (ep->type == EXFAT_ACL)
return TYPE_ACL;
return TYPE_CRITICAL_SEC;
@@ -388,7 +386,7 @@ static void exfat_set_entry_type(struct exfat_dentry *ep,
unsigned int type)
ep->type &= EXFAT_DELETE;
} else if (type == TYPE_STREAM) {
ep->type = EXFAT_STREAM;
- } else if (type == TYPE_EXTEND) {
+ } else if (type == TYPE_NAME) {
ep->type = EXFAT_NAME;
} else if (type == TYPE_BITMAP) {
ep->type = EXFAT_BITMAP;
@@ -421,7 +419,7 @@ static void exfat_init_name_entry(struct exfat_dentry *ep,
{
int i;
- exfat_set_entry_type(ep, TYPE_EXTEND);
+ exfat_set_entry_type(ep, TYPE_NAME);
ep->dentry.name.flags = 0x0;
for (i = 0; i < EXFAT_FILE_NAME_LEN; i++) {
@@ -591,16 +589,13 @@ void exfat_update_dir_chksum_with_entry_set(struct
exfat_entry_set_cache *es)
{
int chksum_type = CS_DIR_ENTRY, i;
unsigned short chksum = 0;
- struct exfat_dentry *ep;
for (i = 0; i < es->num_entries; i++) {
- ep = exfat_get_dentry_cached(es, i);
- chksum = exfat_calc_chksum16(ep, DENTRY_SIZE, chksum,
+ chksum = exfat_calc_chksum16(es->de[i], DENTRY_SIZE, chksum,
chksum_type);
chksum_type = CS_DEFAULT;
}
- ep = exfat_get_dentry_cached(es, 0);
- ep->dentry.file.checksum = cpu_to_le16(chksum);
+ ES_FILE(es).checksum = cpu_to_le16(chksum);
es->modified = true;
}
@@ -741,59 +736,8 @@ struct exfat_dentry *exfat_get_dentry(struct super_block
*sb,
return (struct exfat_dentry *)((*bh)->b_data + off);
}
-enum exfat_validate_dentry_mode {
- ES_MODE_STARTED,
- ES_MODE_GET_FILE_ENTRY,
- ES_MODE_GET_STRM_ENTRY,
- ES_MODE_GET_NAME_ENTRY,
- ES_MODE_GET_CRITICAL_SEC_ENTRY,
-};
-
-static bool exfat_validate_entry(unsigned int type,
- enum exfat_validate_dentry_mode *mode)
-{
- if (type == TYPE_UNUSED || type == TYPE_DELETED)
- return false;
-
- switch (*mode) {
- case ES_MODE_STARTED:
- if (type != TYPE_FILE && type != TYPE_DIR)
- return false;
- *mode = ES_MODE_GET_FILE_ENTRY;
- return true;
- case ES_MODE_GET_FILE_ENTRY:
- if (type != TYPE_STREAM)
- return false;
- *mode = ES_MODE_GET_STRM_ENTRY;
- return true;
- case ES_MODE_GET_STRM_ENTRY:
- if (type != TYPE_EXTEND)
- return false;
-
RE: [PATCH] exfat: use i_blocksize() to get blocksize
> We alreday has the interface i_blocksize() to get blocksize, so use it. > > Signed-off-by: Xianting Tian Pushed it into exfat #dev. Thanks for your patch!
RE: [PATCH v3] exfat: remove EXFAT_SB_DIRTY flag
> Thanks for thinking on this complicated issue. > > > > Most of the NAND flash devices and HDDs have wear leveling and bad sector > > replacement algorithms > applied. > > So I think that the life of the boot sector will not be exhausted first. > > I'm not too worried about the life of the boot-sector. > I'm worried about write failures caused by external factors. > (power failure/system down/vibration/etc. during writing) They rarely occur > on SD cards, but occur on > many HDDs, some SSDs and USB storages by 0.1% or more. Hard disk and SSD do not guarantee atomic write of a sector unit? > Especially with AFT-HDD, not only boot-sector but also the following multiple > sectors become > unreadable. Other file systems will also be unstable on a such HW. > It is not possible to completely solve this problem, as long as writing to > the boot-sector. > (I think it's a exFAT's specification defect) The only effective way to > reduce this problem is to > reduce writes to the boot-sector. exFAT's specification defect... Well.. Even though the boot sector is corrupted, It can be recovered using the backup boot sector through fsck. > > > > Currently the volume dirty/clean policy of exfat-fs is not perfect, > > Thank you for sharing the problem with you. > > > > but I think it behaves similarly to the policy of MS Windows. > > On Windows10, the dirty flag is cleared after more than 15 seconds after all > write operations are > completed. > (dirty-flag is never updated during the write operation continues) > > > > Therefore, > > I think code improvements should be made to reduce volume flag records > > while maintaining the current > policy. > > Current policy is inconsistent. > As I wrote last mail, the problem with the current implementation is that the > dirty-flag may not be > cleared after the write operation.(even if sync is enabled or disabled) > Because, some write operations > clear the dirty-flag but some don't clear. > Unmount or sync command is the only way to ensure that the dirty-flag is > cleared. > This has no effect on clearing the dirty-flag after a write operations, it > only increases risk of > destroying the boot-sector. > - Clear the dirty-flag after every write operation. > - Never clear the dirty-flag after every write operation. > Unless unified to either one, I think that sync policy cannot be consistent. > > How do you think? > > > BR > --- > etsuhiro Kohada
RE: [PATCH 1/2] exfat: add NameLength check when extracting name
> Thank you for your reply.
>
> >> -static void exfat_get_uniname_from_ext_entry(struct super_block *sb,
> >> - struct exfat_chain *p_dir, int entry, unsigned short *uniname)
> >> +static int exfat_get_uniname_from_name_entries(struct
> >> exfat_entry_set_cache *es,
> >> + struct exfat_uni_name *uniname)
> >> {
> >> - int i;
> >> - struct exfat_entry_set_cache *es;
> >> + int n, l, i;
> >>struct exfat_dentry *ep;
> >>
> >> - es = exfat_get_dentry_set(sb, p_dir, entry, ES_ALL_ENTRIES);
> >> - if (!es)
> >> - return;
> >> + uniname->name_len = es->de_stream->name_len;
> >> + if (uniname->name_len == 0)
> >> + return -EIO;
> > Can we validate ->name_len and name entry ->type in exfat_get_dentry_set() ?
>
> Yes.
> As I wrote in a previous email, entry type validation, name-length
> validation, and name extraction
> should not be separated, so implement all of these in exfat_get_dentry_set().
> It can be easily implemented by adding uniname to exfat_entry_set_cache and
> calling
> exfat_get_uniname_from_name_entries() from exfat_get_dentry_set().
No, We can check stream->name_len and name entry type in exfat_get_dentry_set().
And you are already checking entry type with TYPE_SECONDARY in
exfat_get_dentry_set(). Why do we have to check twice?
>
> However, that would be over-implementation.
> Not all callers of exfat_get_dentry_set() need a name.
Where? It will not checked with ES_2_ENTRIES.
> It is enough to validate the name when it is needed.
> This is a file-system driver, not fsck.
Sorry, I don't understand what you are talking about. If there is a problem
in ondisk-metadata, Filesystem should return error.
> Validation is possible in exfat_get_dentry_set(), but unnecessary.
>
> Why do you want to validate the name in exfat_get_dentry_set()?
exfat_get_dentry_set validates file, stream entry. And you are trying to check
name entries with type_secondary. In addition, trying add the checksum check.
Conversely, Why would you want to add those checks to exfat_get_dentry_set()?
Why do we check only name entries separately? Aren't you intent to return
validated entry set in exfat_get_dentry_set()?
>
>
> BR
> ---
> Tetsuhiro Kohada
[GIT PULL] exfat update for 5.9-rc1
Hi Linus, This is exfat update pull request for v5.9-rc1. I add description of this pull request on below. Please pull exfat with following ones. Thanks! The following changes since commit bcf876870b95592b52519ed4aafcf9d95999bc9c: Linux 5.8 (2020-08-02 14:21:45 -0700) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/linkinjeon/exfat.git tags/exfat-for-5.9-rc1 for you to fetch changes up to 7018ec68f08249de17cb131b324d5a48e89ed898: exfat: retain 'VolumeFlags' properly (2020-08-12 08:31:13 +0900) Description for this pull request: - Don't clear MediaFailure and VolumeDirty bit in volume flags if these were already set before mounting. - Write multiple dirty buffers at once in sync mode. - Remove unneeded EXFAT_SB_DIRTY bit set. Tetsuhiro Kohada (5): exfat: remove EXFAT_SB_DIRTY flag exfat: write multiple sectors at once exfat: add error check when updating dir-entries exfat: optimize exfat_zeroed_cluster() exfat: retain 'VolumeFlags' properly fs/exfat/balloc.c| 4 ++-- fs/exfat/dir.c | 32 - fs/exfat/exfat_fs.h | 14 ++--- fs/exfat/exfat_raw.h | 5 ++--- fs/exfat/fatent.c| 58 ++-- fs/exfat/file.c | 9 +--- fs/exfat/inode.c | 13 ++-- fs/exfat/misc.c | 22 ++-- fs/exfat/namei.c | 32 ++--- fs/exfat/super.c | 48 --- 10 files changed, 121 insertions(+), 116 deletions(-)
RE: [PATCH 1/2] exfat: add dir-entry set checksum validation
> Add checksum validation for dir-entry set when getting it.
> exfat_calc_dir_chksum_with_entry_set() also validates entry-type.
>
> ** This patch depends on:
> '[PATCH v3] exfat: integrates dir-entry getting and validation'
>
> Signed-off-by: Tetsuhiro Kohada
> ---
> fs/exfat/dir.c | 34 ++
> 1 file changed, 22 insertions(+), 12 deletions(-)
>
> diff --git a/fs/exfat/dir.c b/fs/exfat/dir.c index c9715c7a55a1..2e79ac464f5f
> 100644
> --- a/fs/exfat/dir.c
> +++ b/fs/exfat/dir.c
> @@ -563,18 +563,27 @@ int exfat_remove_entries(struct inode *inode, struct
> exfat_chain *p_dir,
> return 0;
> }
>
> -void exfat_update_dir_chksum_with_entry_set(struct exfat_entry_set_cache *es)
> +static int exfat_calc_dir_chksum_with_entry_set(struct
> +exfat_entry_set_cache *es, u16 *chksum)
> {
> - int chksum_type = CS_DIR_ENTRY, i;
> - unsigned short chksum = 0;
> struct exfat_dentry *ep;
> + int i;
>
> - for (i = 0; i < es->num_entries; i++) {
> - ep = exfat_get_validated_dentry(es, i, TYPE_ALL);
> - chksum = exfat_calc_chksum16(ep, DENTRY_SIZE, chksum,
> - chksum_type);
> - chksum_type = CS_DEFAULT;
> + ep = container_of(es->de_file, struct exfat_dentry, dentry.file);
> + *chksum = exfat_calc_chksum16(ep, DENTRY_SIZE, 0, CS_DIR_ENTRY);
> + for (i = 0; i < es->de_file->num_ext; i++) {
> + ep = exfat_get_validated_dentry(es, 1 + i, TYPE_SECONDARY);
> + if (!ep)
> + return -EIO;
> + *chksum = exfat_calc_chksum16(ep, DENTRY_SIZE, *chksum,
> CS_DEFAULT);
> }
> + return 0;
We can return checksum after removing u16 *chksum argument.
> +}
> +
> +void exfat_update_dir_chksum_with_entry_set(struct
> +exfat_entry_set_cache *es) {
> + u16 chksum;
> +
> + exfat_calc_dir_chksum_with_entry_set(es, );
> es->de_file->checksum = cpu_to_le16(chksum);
> es->modified = true;
> }
> @@ -775,6 +784,7 @@ struct exfat_entry_set_cache *exfat_get_dentry_set(struct
> super_block *sb,
> struct exfat_entry_set_cache *es;
> struct exfat_dentry *ep;
> struct buffer_head *bh;
> + u16 chksum;
>
> if (p_dir->dir == DIR_DELETED) {
> exfat_err(sb, "access to deleted dentry"); @@ -839,10 +849,10
> @@ struct
> exfat_entry_set_cache *exfat_get_dentry_set(struct super_block *sb,
> goto free_es;
> es->de_stream = >dentry.stream;
>
> - for (i = 2; i < es->num_entries; i++) {
> - if (!exfat_get_validated_dentry(es, i, TYPE_SECONDARY))
> - goto free_es;
> - }
> + if (max_entries == ES_ALL_ENTRIES &&
> + ((exfat_calc_dir_chksum_with_entry_set(es, ) ||
> + chksum != le16_to_cpu(es->de_file->checksum
Please add error print log if checksum mismatch error happen.
> + goto free_es;
>
> return es;
>
> --
> 2.25.1
RE: [PATCH 1/2] exfat: add NameLength check when extracting name
> The current implementation doesn't care NameLength when extracting the name
> from Name dir-entries, so
> the name may be incorrect.
> (Without null-termination, Insufficient Name dir-entry, etc) Add a NameLength
> check when extracting
> the name from Name dir-entries to extract correct name.
> And, change to get the information of file/stream-ext dir-entries via the
> member variable of
> exfat_entry_set_cache.
>
> ** This patch depends on:
> '[PATCH v3] exfat: integrates dir-entry getting and validation'.
>
> Signed-off-by: Tetsuhiro Kohada
> ---
> fs/exfat/dir.c | 81 --
> 1 file changed, 39 insertions(+), 42 deletions(-)
>
> diff --git a/fs/exfat/dir.c b/fs/exfat/dir.c index 91cdbede0fd1..545bb73b95e9
> 100644
> --- a/fs/exfat/dir.c
> +++ b/fs/exfat/dir.c
> @@ -28,16 +28,15 @@ static int exfat_extract_uni_name(struct exfat_dentry *ep,
>
> }
>
> -static void exfat_get_uniname_from_ext_entry(struct super_block *sb,
> - struct exfat_chain *p_dir, int entry, unsigned short *uniname)
> +static int exfat_get_uniname_from_name_entries(struct exfat_entry_set_cache
> *es,
> + struct exfat_uni_name *uniname)
> {
> - int i;
> - struct exfat_entry_set_cache *es;
> + int n, l, i;
> struct exfat_dentry *ep;
>
> - es = exfat_get_dentry_set(sb, p_dir, entry, ES_ALL_ENTRIES);
> - if (!es)
> - return;
> + uniname->name_len = es->de_stream->name_len;
> + if (uniname->name_len == 0)
> + return -EIO;
Can we validate ->name_len and name entry ->type in exfat_get_dentry_set() ?
>
> /*
>* First entry : file entry
> @@ -45,14 +44,15 @@ static void exfat_get_uniname_from_ext_entry(struct
> super_block *sb,
>* Third entry : first file-name entry
>* So, the index of first file-name dentry should start from 2.
>*/
> -
> - i = 2;
> - while ((ep = exfat_get_validated_dentry(es, i++, TYPE_NAME))) {
> - exfat_extract_uni_name(ep, uniname);
> - uniname += EXFAT_FILE_NAME_LEN;
> + for (l = 0, n = 2; l < uniname->name_len; n++) {
> + ep = exfat_get_validated_dentry(es, n, TYPE_NAME);
> + if (!ep)
> + return -EIO;
> + for (i = 0; l < uniname->name_len && i < EXFAT_FILE_NAME_LEN;
> i++, l++)
> + uniname->name[l] =
> le16_to_cpu(ep->dentry.name.unicode_0_14[i]);
> }
> -
> - exfat_free_dentry_set(es, false);
> + uniname->name[l] = 0;
> + return 0;
> }
RE: [PATCH v3] exfat: integrates dir-entry getting and validation
>
> +#define TYPE_PRIMARY (TYPE_CRITICAL_PRI | TYPE_BENIGN_PRI)
> +#define TYPE_SECONDARY (TYPE_CRITICAL_SEC | TYPE_BENIGN_SEC)
> +
> #define MAX_CHARSET_SIZE 6 /* max size of multi-byte character */
> #define MAX_NAME_LENGTH 255 /* max len of file name excluding
> NULL */
> #define MAX_VFSNAME_BUF_SIZE ((MAX_NAME_LENGTH + 1) * MAX_CHARSET_SIZE)
> @@ -171,7 +174,9 @@ struct exfat_entry_set_cache {
> unsigned int start_off;
> int num_bh;
> struct buffer_head *bh[DIR_CACHE_SIZE];
> - unsigned int num_entries;
> + int num_entries;
> + struct exfat_de_file *de_file;
> + struct exfat_de_stream *de_stream;
I prefer to assign validated entries to **de and use it using enum value.
struct exfat_dentry **de;
> };
RE: [PATCH v2] exfat: integrates dir-entry getting and validation
> Thank you for your reply.
>
> > > diff --git a/fs/exfat/dir.c b/fs/exfat/dir.c index
> > > 573659bfbc55..09b85746e760 100644
> > > --- a/fs/exfat/dir.c
> > > +++ b/fs/exfat/dir.c
> > > @@ -33,6 +33,7 @@ static void exfat_get_uniname_from_ext_entry(struct
> > > super_block *sb, {
> > > int i;
> > > struct exfat_entry_set_cache *es;
> > > + struct exfat_dentry *ep;
> > >
> > > es = exfat_get_dentry_set(sb, p_dir, entry, ES_ALL_ENTRIES);
> > > if (!es)
> > > @@ -44,13 +45,9 @@ static void exfat_get_uniname_from_ext_entry(struct
> > > super_block *sb,
> > >* Third entry : first file-name entry
> > >* So, the index of first file-name dentry should start from 2.
> > >*/
> > > - for (i = 2; i < es->num_entries; i++) {
> > > - struct exfat_dentry *ep = exfat_get_dentry_cached(es, i);
> > > -
> > > - /* end of name entry */
> > > - if (exfat_get_entry_type(ep) != TYPE_EXTEND)
> > > - break;
> > >
> > > + i = 2;
> > > + while ((ep = exfat_get_validated_dentry(es, i++, TYPE_NAME))) {
> > As Sungjong said, I think that TYPE_NAME seems right to be validated in
> > exfat_get_dentry_set().
>
> First, it is possible to correctly determine that "Immediately follow the
> Stream Extension directory
> entry as a consecutive series"
> whether the TYPE_NAME check is implemented here or exfat_get_dentry_set().
> It's functionally same, so it is also right to validate in either.
>
> Second, the current implementation does not care for NameLength field, as I
> replied to Sungjong.
> If name is not terminated with zero, the name will be incorrect.(With or
> without my patch) I think
> TYPE_NAME and NameLength validation should not be separated from the name
> extraction.
> If validate TYPE_NAME in exfat_get_dentry_set(), NameLength validation and
> name extraction should also
> be implemented there.
> (Otherwise, a duplication check with exfat_get_dentry_set() and here.) I will
> add NameLength
> validation here.
Okay.
> Therefore, TYPE_NAME validation here should not be omitted.
>
> Third, getting dentry and entry-type validation should be integrated.
> These no longer have to be primitive.
> The integration simplifies caller error checking.
>
>
> > > -struct exfat_dentry *exfat_get_dentry_cached(
> > > - struct exfat_entry_set_cache *es, int num)
> > > +struct exfat_dentry *exfat_get_validated_dentry(struct
> > > exfat_entry_set_cache *es,
> > > + int num, unsigned int type)
> > Please use two tabs.
>
> OK.
> I'll fix it.
>
>
> > > + struct buffer_head *bh;
> > > + struct exfat_dentry *ep;
> > >
> > > - return (struct exfat_dentry *)p;
> > > + if (num >= es->num_entries)
> > > + return NULL;
> > > +
> > > + bh = es->bh[EXFAT_B_TO_BLK(off, es->sb)];
> > > + if (!bh)
> > > + return NULL;
> > > +
> > > + ep = (struct exfat_dentry *)
> > > + (bh->b_data + EXFAT_BLK_OFFSET(off, es->sb));
> > > +
> > > + switch (type) {
> > > + case TYPE_ALL: /* accept any */
> > > + break;
> > > + case TYPE_FILE:
> > > + if (ep->type != EXFAT_FILE)
> > > + return NULL;
> > > + break;
> > > + case TYPE_SECONDARY:
> > > + if (!(type & exfat_get_entry_type(ep)))
> > > + return NULL;
> > > + break;
> > Type check should be in this order :
> > FILE->STREAM->NAME->{CRITICAL_SEC|BENIGN_SEC}
> > I think that you are missing TYPE_NAME check here.
>
> Types other than the above (TYPE_NAME, TYPE_STREAM, etc.) are checked in the
> default-case(as below).
>
> > > + default:
> > > + if (type != exfat_get_entry_type(ep))
> > > + return NULL;
> > > + }
> > > + return ep;
> > > }
> > >
> > > /*
> > > * Returns a set of dentries for a file or dir.
> > > *
> > > - * Note It provides a direct pointer to bh->data via
> > > exfat_get_dentry_cached().
> > > + * Note It provides a direct pointer to bh->data via
> > > exfat_get_validated_dentry().
> > > * User should call exfat_get_dentry_set() after setting 'modified' to
> > > apply
> > > * changes made in this entry set to the real device.
> > > *
> > > * in:
> > > * sb+p_dir+entry: indicates a file/dir
> > > - * type: specifies how many dentries should be included.
> > > + * max_entries: specifies how many dentries should be included.
> > > * return:
> > > * pointer of entry set on success,
> > > * NULL on failure.
> > > + * note:
> > > + * On success, guarantee the correct 'file' and 'stream-ext'
> > > dir-entries.
> > This comment seems unnecessary.
>
> I'll remove it.
>
> > > diff --git a/fs/exfat/file.c b/fs/exfat/file.c index
> > > 6707f3eb09b5..b6b458e6f5e3 100644
> > > --- a/fs/exfat/file.c
> > > +++ b/fs/exfat/file.c
> > > @@ -160,8 +160,8 @@ int __exfat_truncate(struct inode *inode, loff_t
> > > new_size)
> > > ES_ALL_ENTRIES);
> > > if (!es)
> > > return -EIO;
> > > -
Re: exfatprogs-1.0.4 version released
2020-07-31 18:56 GMT+09:00, Sedat Dilek : > On Fri, Jul 31, 2020 at 9:16 AM Namjae Jeon > wrote: >> >> Hi folk, >> >> In this release, The performance of fsck have been much improved and >> the new option in mkfs have been added to adjust boundary alignment. >> >> As the result below, The fsck performance is improved close to windows's >> fsck >> and much faster than the one in exfat-utils package. >> >> We measured the performance on Samsung 64GB Pro microSDXC UHS-I Class 10 >> which >> was filled up to 35GB with 9948 directories and 16506 files. >> >> | Implementation | version | execution time (seconds) | >> |- |---|--| >> | **exfatprogs fsck** | 1.0.4 | 11.561 | >> | Windows fsck | Windows 10 1809 | 11.449 | >> | [exfat-fuse fsck]| 1.3.0 | 68.977 | >> > > Hi Namjae Jeon (what is your First name and Family name?), Hi Sedat, First name : Namjae, Last name : Jeon > > congrats to version 1.0.4 and hope to see it in the Debian repositories > soon. Thanks:) > > Great numbers for exfatprogs/fsck! > >> And we have been preparing to add fsck repair feature in the next >> version. >> Any feedback is welcome!:) >> >> CHANGES : >> * fsck.exfat: display sector, cluster, and volume sizes in the human >>readable format. >> * fsck.exfat: reduce the elapsed time using read-ahead. >> >> NEW FEATURES : >> * mkfs.exfat: generate pseudo unique serials while creating filesystems. >> * mkfs.exfat: add the "-b" option to align the start offset of FAT and >>data clusters. >> * fsck.exfat: repair zero-byte files which have the NoFatChain >> attribute. >> >> BUG FIXES : >> * Fix memory leaks on error handling paths. >> * fsck.exfat: fix the bug that cannot access space beyond 2TB. >> >> The git tree is at: >> https://github.com/exfatprogs/exfatprogs >> >> The tarballs can be found at: >> >> https://github.com/exfatprogs/exfatprogs/releases/download/1.0.4/exfatprogs-1.0.4.tar.gz >> > > Just a small nit for the next announcement. > Please point (also) to the tar.xz tarball and maybe to the releases > tag in GitHub. Sure. I will do that on next release. > ( Today, I love/prefer to use ZSTD where possible - that is another story. > ) I'm not sure if it is meaningful to provide various compression for small archive file. In particular, I wonder if other distribution maintainers want it. > > Have more fun. Nice day! > > Regards, > - Sedat - > > [1] > https://github.com/exfatprogs/exfatprogs/releases/download/1.0.4/exfatprogs-1.0.4.tar.xz > [2] https://github.com/exfatprogs/exfatprogs/releases/tag/1.0.4 >
exfatprogs-1.0.4 version released
Hi folk, In this release, The performance of fsck have been much improved and the new option in mkfs have been added to adjust boundary alignment. As the result below, The fsck performance is improved close to windows's fsck and much faster than the one in exfat-utils package. We measured the performance on Samsung 64GB Pro microSDXC UHS-I Class 10 which was filled up to 35GB with 9948 directories and 16506 files. | Implementation | version | execution time (seconds) | |- |---|--| | **exfatprogs fsck** | 1.0.4 | 11.561 | | Windows fsck | Windows 10 1809 | 11.449 | | [exfat-fuse fsck]| 1.3.0 | 68.977 | And we have been preparing to add fsck repair feature in the next version. Any feedback is welcome!:) CHANGES : * fsck.exfat: display sector, cluster, and volume sizes in the human readable format. * fsck.exfat: reduce the elapsed time using read-ahead. NEW FEATURES : * mkfs.exfat: generate pseudo unique serials while creating filesystems. * mkfs.exfat: add the "-b" option to align the start offset of FAT and data clusters. * fsck.exfat: repair zero-byte files which have the NoFatChain attribute. BUG FIXES : * Fix memory leaks on error handling paths. * fsck.exfat: fix the bug that cannot access space beyond 2TB. The git tree is at: https://github.com/exfatprogs/exfatprogs The tarballs can be found at: https://github.com/exfatprogs/exfatprogs/releases/download/1.0.4/exfatprogs-1.0.4.tar.gz
RE: [PATCH] exfat: retain 'VolumeFlags' properly
> Ping.. Hi Tetsuhiro, > > On 2020/07/15 19:06, Tetsuhiro Kohada wrote: > >> It looks complicated. It would be better to simply set/clear VOLUME DIRTY > >> bit. > > > > I think exfat_set_vol_flags() gets a little complicated, because it > > needs the followings (with bit operation) > > a) Set/Clear VOLUME_DIRTY. > > b) Set MEDIA_FAILUR. > > How about splitting these into separate functions as below? > > > exfat_set_volume_dirty() > exfat_set_vol_flags(sb, sbi->vol_flag | VOLUME_DIRTY); > > exfat_clear_volume_dirty() > exfat_set_vol_flags(sb, sbi->vol_flag & ~VOLUME_DIRTY); Looks good. > > exfat_set_media_failure() > exfat_set_vol_flags(sb, sbi->vol_flag | MEDIA_FAILURE); Where will this function be called? We don't need to create unused functions in advance... > > > The implementation is essentially the same for exfat_set_vol_flags(), but I > think the intention of the > operation will be easier to understand. Yes. Thanks! > > > BR > --- > Tetsuhiro Kohada
RE: [PATCH v2] exfat: integrates dir-entry getting and validation
> Add validation for num, bh and type on getting dir-entry.
> ('file' and 'stream-ext' dir-entries are pre-validated to ensure success)
> Renamed
> exfat_get_dentry_cached() to exfat_get_validated_dentry() due to a change in
> functionality.
>
> Integrate type-validation with simplified.
> This will also recognize a dir-entry set that contains 'benign secondary'
> dir-entries.
>
> And, rename TYPE_EXTEND to TYPE_NAME.
>
> Suggested-by: Sungjong Seo
> Signed-off-by: Tetsuhiro Kohada
> ---
> Changes in v2
> - Change verification order
> - Verification loop start with index 2
>
> fs/exfat/dir.c | 144 ++--
> fs/exfat/exfat_fs.h | 15 +++--
> fs/exfat/file.c | 4 +-
> fs/exfat/inode.c| 6 +-
> fs/exfat/namei.c| 4 +-
> 5 files changed, 73 insertions(+), 100 deletions(-)
>
> diff --git a/fs/exfat/dir.c b/fs/exfat/dir.c index 573659bfbc55..09b85746e760
> 100644
> --- a/fs/exfat/dir.c
> +++ b/fs/exfat/dir.c
> @@ -33,6 +33,7 @@ static void exfat_get_uniname_from_ext_entry(struct
> super_block *sb, {
> int i;
> struct exfat_entry_set_cache *es;
> + struct exfat_dentry *ep;
>
> es = exfat_get_dentry_set(sb, p_dir, entry, ES_ALL_ENTRIES);
> if (!es)
> @@ -44,13 +45,9 @@ static void exfat_get_uniname_from_ext_entry(struct
> super_block *sb,
>* Third entry : first file-name entry
>* So, the index of first file-name dentry should start from 2.
>*/
> - for (i = 2; i < es->num_entries; i++) {
> - struct exfat_dentry *ep = exfat_get_dentry_cached(es, i);
> -
> - /* end of name entry */
> - if (exfat_get_entry_type(ep) != TYPE_EXTEND)
> - break;
>
> + i = 2;
> + while ((ep = exfat_get_validated_dentry(es, i++, TYPE_NAME))) {
As Sungjong said, I think that TYPE_NAME seems right to be validated in
exfat_get_dentry_set().
> exfat_extract_uni_name(ep, uniname);
> uniname += EXFAT_FILE_NAME_LEN;
> }
> @@ -372,7 +369,7 @@ unsigned int exfat_get_entry_type(struct exfat_dentry *ep)
> if (ep->type == EXFAT_STREAM)
> return TYPE_STREAM;
> if (ep->type == EXFAT_NAME)
> - return TYPE_EXTEND;
> + return TYPE_NAME;
> if (ep->type == EXFAT_ACL)
> return TYPE_ACL;
> return TYPE_CRITICAL_SEC;
> @@ -388,7 +385,7 @@ static void exfat_set_entry_type(struct exfat_dentry *ep,
> unsigned int type)
> ep->type &= EXFAT_DELETE;
> } else if (type == TYPE_STREAM) {
> ep->type = EXFAT_STREAM;
> - } else if (type == TYPE_EXTEND) {
> + } else if (type == TYPE_NAME) {
> ep->type = EXFAT_NAME;
> } else if (type == TYPE_BITMAP) {
> ep->type = EXFAT_BITMAP;
> @@ -421,7 +418,7 @@ static void exfat_init_name_entry(struct exfat_dentry
> *ep, {
> int i;
>
> - exfat_set_entry_type(ep, TYPE_EXTEND);
> + exfat_set_entry_type(ep, TYPE_NAME);
> ep->dentry.name.flags = 0x0;
>
> for (i = 0; i < EXFAT_FILE_NAME_LEN; i++) { @@ -594,12 +591,12 @@ void
> exfat_update_dir_chksum_with_entry_set(struct exfat_entry_set_cache *es)
> struct exfat_dentry *ep;
>
> for (i = 0; i < es->num_entries; i++) {
> - ep = exfat_get_dentry_cached(es, i);
> + ep = exfat_get_validated_dentry(es, i, TYPE_ALL);
> chksum = exfat_calc_chksum16(ep, DENTRY_SIZE, chksum,
>chksum_type);
> chksum_type = CS_DEFAULT;
> }
> - ep = exfat_get_dentry_cached(es, 0);
> + ep = exfat_get_validated_dentry(es, 0, TYPE_FILE);
> ep->dentry.file.checksum = cpu_to_le16(chksum);
> es->modified = true;
> }
> @@ -741,92 +738,66 @@ struct exfat_dentry *exfat_get_dentry(struct
> super_block *sb,
> return (struct exfat_dentry *)((*bh)->b_data + off); }
>
> -enum exfat_validate_dentry_mode {
> - ES_MODE_STARTED,
> - ES_MODE_GET_FILE_ENTRY,
> - ES_MODE_GET_STRM_ENTRY,
> - ES_MODE_GET_NAME_ENTRY,
> - ES_MODE_GET_CRITICAL_SEC_ENTRY,
> -};
> -
> -static bool exfat_validate_entry(unsigned int type,
> - enum exfat_validate_dentry_mode *mode)
> -{
> - if (type == TYPE_UNUSED || type == TYPE_DELETED)
> - return false;
> -
> - switch (*mode) {
> - case ES_MODE_STARTED:
> - if (type != TYPE_FILE && type != TYPE_DIR)
> - return false;
> - *mode = ES_MODE_GET_FILE_ENTRY;
> - return true;
> - case ES_MODE_GET_FILE_ENTRY:
> - if (type != TYPE_STREAM)
> - return false;
> - *mode = ES_MODE_GET_STRM_ENTRY;
> - return true;
> - case ES_MODE_GET_STRM_ENTRY:
> - if (type != TYPE_EXTEND)
> - return false;
> -
[GIT PULL] exfat fixes for 5.8-rc7
Hi Linus, This is exfat fixes pull request for v5.8-rc7. I add description of this pull request on below. Please pull exfat with following fixes. Thanks! The following changes since commit ba47d845d715a010f7b51f6f89bae32845e6acb7: Linux 5.8-rc6 (2020-07-19 15:41:18 -0700) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/linkinjeon/exfat.git tags/exfat-for-5.8-rc7 for you to fetch changes up to db415f7aae07cadcabd5d2a659f8ad825c905299: exfat: fix name_hash computation on big endian systems (2020-07-21 10:44:19 +0900) Description for this pull request: - fix overflow issue at sector calculation. - fix wrong hint_stat initialization. - fix wrong size update of stream entry. - fix endianness of upname in name_hash computation. Hyeongseok Kim (1): exfat: fix wrong size update of stream entry by typo Ilya Ponetayev (1): exfat: fix name_hash computation on big endian systems Namjae Jeon (2): exfat: fix overflow issue in exfat_cluster_to_sector() exfat: fix wrong hint_stat initialization in exfat_find_dir_entry() fs/exfat/dir.c | 2 +- fs/exfat/exfat_fs.h | 2 +- fs/exfat/file.c | 2 +- fs/exfat/nls.c | 8 4 files changed, 7 insertions(+), 7 deletions(-)
RE: [PATCH] exfat: retain 'VolumeFlags' properly
> Thanks for your reply.
>
> > > Also, rename ERR_MEDIUM to MED_FAILURE.
> > I think that MEDIA_FAILURE looks better.
>
> I think so too.
> If so, should I change VOL_DIRTY to VOLUME_DIRTY?
Yes, maybe.
>
> > > -int exfat_set_vol_flags(struct super_block *sb, unsigned short
> > > new_flag)
> > > +int exfat_set_vol_flags(struct super_block *sb, unsigned short
> > > +new_flags)
> > > {
> > > struct exfat_sb_info *sbi = EXFAT_SB(sb);
> > > struct boot_sector *p_boot = (struct boot_sector *)sbi->boot_bh->b_data;
> > > bool sync;
> > If dirty bit is set in on-disk volume flags, we can just return 0 at the
> > beginning of this function ?
>
> That's right.
> Neither 'set VOL_DIRTY' nor 'set VOL_CLEAN' makes a change to volume flags.
>
>
> > > + if (new_flags == VOL_CLEAN)
> > > + new_flags = (sbi->vol_flags & ~VOL_DIRTY) |
> > > sbi->vol_flags_noclear;
> > > + else
> > > + new_flags |= sbi->vol_flags;
> > > +
> > > /* flags are not changed */
> > > - if (sbi->vol_flag == new_flag)
> > > + if (sbi->vol_flags == new_flags)
> > > return 0;
> > >
> > > - sbi->vol_flag = new_flag;
> > > + sbi->vol_flags = new_flags;
> > >
> > > /* skip updating volume dirty flag,
> > >* if this volume has been mounted with read-only @@ -114,9 +119,9
> > > @@ int exfat_set_vol_flags(struct super_block *sb, unsigned short
> > > new_flag)
> > > if (sb_rdonly(sb))
> > > return 0;
> > >
> > > - p_boot->vol_flags = cpu_to_le16(new_flag);
> > > + p_boot->vol_flags = cpu_to_le16(new_flags);
> > How about set or clear only dirty bit to on-disk volume flags instead
> > of using
> > sbi->vol_flags_noclear ?
> >if (set)
> >p_boot->vol_flags |= cpu_to_le16(VOL_DIRTY);
> >else
> >p_boot->vol_flags &= cpu_to_le16(~VOL_DIRTY);
>
> In this way, the initial VOL_DIRTY cannot be retained.
> The purpose of this patch is to avoid losing the initial VOL_DIRTY and
> MED_FAILURE.
> Furthermore, I'm also thinking of setting MED_FAILURE.
I know what you do. I mean this function does not need to be called if volume
dirty
Is already set on on-disk volume flags as I said earlier.
>
> However, the formula for 'new_flags' may be a bit complicated.
> Is it better to change to the following?
>
> if (new_flags == VOL_CLEAN)
> new_flags = sbi->vol_flags & ~VOL_DIRTY
> else
> new_flags |= sbi->vol_flags;
>
> new_flags |= sbi->vol_flags_noclear;
>
>
> one more point,
> Is there a better name than 'vol_flags_noclear'?
> (I can't come up with a good name anymore)
It looks complicated. It would be better to simply set/clear VOLUME DIRTY bit.
Thanks!
>
> BR
> ---
> Kohada Tetsuhiro
RE: [PATCH] exfat: retain 'VolumeFlags' properly
Hi Tetsuhiro,
> Retain ActiveFat, MediaFailure and ClearToZero fields.
> And, never clear VolumeDirty, if it is dirty at mount.
>
> In '3.1.13.3 Media Failure Field' of exfat specification says ...
> If, upon mounting a volume, the value of this field is 1, implementations
> which scan the entire
> volume for media failures and record all failures as "bad" clusters in the
> FAT (or otherwise resolve
> media failures) may clear the value of this field to 0.
> Therefore, should not clear MediaFailure without scanning volume.
>
> In '8.1 Recommended Write Ordering' of exfat specification says ...
> Clear the value of the VolumeDirty field to 0, if its value prior to the
> first step was 0 Therefore,
> should not clear VolumeDirty when mounted.
>
> Also, rename ERR_MEDIUM to MED_FAILURE.
I think that MEDIA_FAILURE looks better.
>
> Signed-off-by: Tetsuhiro Kohada
> ---
> fs/exfat/exfat_fs.h | 5 +++--
> fs/exfat/exfat_raw.h | 2 +-
> fs/exfat/super.c | 22 ++
> 3 files changed, 18 insertions(+), 11 deletions(-)
>
> diff --git a/fs/exfat/exfat_fs.h b/fs/exfat/exfat_fs.h index
> cb51d6e83199..3f8dc4ca8109 100644
> --- a/fs/exfat/exfat_fs.h
> +++ b/fs/exfat/exfat_fs.h
> @@ -224,7 +224,8 @@ struct exfat_sb_info {
> unsigned int num_FAT_sectors; /* num of FAT sectors */
> unsigned int root_dir; /* root dir cluster */
> unsigned int dentries_per_clu; /* num of dentries per cluster */
> - unsigned int vol_flag; /* volume dirty flag */
> + unsigned int vol_flags; /* volume flags */
> + unsigned int vol_flags_noclear; /* volume flags to retain */
> struct buffer_head *boot_bh; /* buffer_head of BOOT sector */
>
> unsigned int map_clu; /* allocation bitmap start cluster */ @@ -380,7
> +381,7 @@ static inline
> int exfat_sector_to_cluster(struct exfat_sb_info *sbi, }
>
> /* super.c */
> -int exfat_set_vol_flags(struct super_block *sb, unsigned short new_flag);
> +int exfat_set_vol_flags(struct super_block *sb, unsigned short
> +new_flags);
>
> /* fatent.c */
> #define exfat_get_next_cluster(sb, pclu) exfat_ent_get(sb, *(pclu), pclu)
> diff --git
> a/fs/exfat/exfat_raw.h b/fs/exfat/exfat_raw.h index
> 350ce59cc324..d86a8a6b0601 100644
> --- a/fs/exfat/exfat_raw.h
> +++ b/fs/exfat/exfat_raw.h
> @@ -16,7 +16,7 @@
>
> #define VOL_CLEAN0x
> #define VOL_DIRTY0x0002
> -#define ERR_MEDIUM 0x0004
> +#define MED_FAILURE 0x0004
>
> #define EXFAT_EOF_CLUSTER0xu
> #define EXFAT_BAD_CLUSTER0xFFF7u
> diff --git a/fs/exfat/super.c b/fs/exfat/super.c index
> b5bf6dedbe11..c26b0f5a0875 100644
> --- a/fs/exfat/super.c
> +++ b/fs/exfat/super.c
> @@ -96,17 +96,22 @@ static int exfat_statfs(struct dentry *dentry, struct
> kstatfs *buf)
> return 0;
> }
>
> -int exfat_set_vol_flags(struct super_block *sb, unsigned short new_flag)
> +int exfat_set_vol_flags(struct super_block *sb, unsigned short
> +new_flags)
> {
> struct exfat_sb_info *sbi = EXFAT_SB(sb);
> struct boot_sector *p_boot = (struct boot_sector *)sbi->boot_bh->b_data;
> bool sync;
If dirty bit is set in on-disk volume flags, we can just return 0 at the
beginning
of this function ?
>
> + if (new_flags == VOL_CLEAN)
> + new_flags = (sbi->vol_flags & ~VOL_DIRTY) |
> sbi->vol_flags_noclear;
> + else
> + new_flags |= sbi->vol_flags;
> +
> /* flags are not changed */
> - if (sbi->vol_flag == new_flag)
> + if (sbi->vol_flags == new_flags)
> return 0;
>
> - sbi->vol_flag = new_flag;
> + sbi->vol_flags = new_flags;
>
> /* skip updating volume dirty flag,
>* if this volume has been mounted with read-only @@ -114,9 +119,9 @@
> int
> exfat_set_vol_flags(struct super_block *sb, unsigned short new_flag)
> if (sb_rdonly(sb))
> return 0;
>
> - p_boot->vol_flags = cpu_to_le16(new_flag);
> + p_boot->vol_flags = cpu_to_le16(new_flags);
How about set or clear only dirty bit to on-disk volume flags instead of using
sbi->vol_flags_noclear ?
if (set)
p_boot->vol_flags |= cpu_to_le16(VOL_DIRTY);
else
p_boot->vol_flags &= cpu_to_le16(~VOL_DIRTY);
>
> - if (new_flag == VOL_DIRTY && !buffer_dirty(sbi->boot_bh))
> + if ((new_flags & VOL_DIRTY) && !buffer_dirty(sbi->boot_bh))
> sync = true;
> else
> sync = false;
> @@ -457,7 +462,8 @@ static int exfat_read_boot_sector(struct super_block *sb)
> sbi->dentries_per_clu = 1 <<
> (sbi->cluster_size_bits - DENTRY_SIZE_BITS);
>
> - sbi->vol_flag = le16_to_cpu(p_boot->vol_flags);
> + sbi->vol_flags = le16_to_cpu(p_boot->vol_flags);
> + sbi->vol_flags_noclear = sbi->vol_flags & (VOL_DIRTY | MED_FAILURE);
> sbi->clu_srch_ptr = EXFAT_FIRST_CLUSTER;
> sbi->used_clusters = EXFAT_CLUSTERS_UNTRACKED;
>
> @@ -472,9 +478,9 @@ static int
RE: [PATCH] exfat: fix wrong size update of stream entry by typo
> The stream.size field is updated to the value of create timestamp of the file
> entry. Fix this to use
> correct stream entry pointer.
>
> Fixes: 29bbb14bfc80 ("exfat: fix incorrect update of stream entry in
> __exfat_truncate()")
> Signed-off-by: Hyeongseok Kim
My bad, Pushed it into exfat #dev.
Thanks!
Re: [PATCH] exfat: implement "quiet" option for setattr
2020-07-06 17:22 GMT+09:00, Ju Hyung Park :
> Hi Namjae.
Hi Juhyung,
>
> Looks like I ported this incorrectly from the previous sdFAT base.
>
> I'll fix, test again and send v2.
Okay:) Thanks!
>
> Thanks.
>
> On Thu, Jul 2, 2020 at 2:16 PM Namjae Jeon wrote:
>>
>> >
>> > if (((attr->ia_valid & ATTR_UID) &&
>> >!uid_eq(attr->ia_uid, sbi->options.fs_uid)) || @@ -322,6
>> > +325,12 @@ int
>> > exfat_setattr(struct dentry *dentry, struct iattr *attr)
>> > goto out;
>> You should remove goto statement and curly braces here to reach if error
>> condition.
>> > }
>> >
>> > + if (error) {
>> > + if (sbi->options.quiet)
>> > + error = 0;
>> > + goto out;
>> > + }
>>
>
RE: [PATCH v2] exfat: optimize exfat_zeroed_cluster()
> > Replace part of exfat_zeroed_cluster() with exfat_update_bhs(). > > And remove exfat_sync_bhs(). > > > > Signed-off-by: Tetsuhiro Kohada > > Reviewed-by: Sungjong Seo > > Looks good. Thanks. Pushed it into exfat #dev. Thanks!
RE: [PATCH] exfat: implement "quiet" option for setattr
>
> if (((attr->ia_valid & ATTR_UID) &&
>!uid_eq(attr->ia_uid, sbi->options.fs_uid)) || @@ -322,6 +325,12
> @@ int
> exfat_setattr(struct dentry *dentry, struct iattr *attr)
> goto out;
You should remove goto statement and curly braces here to reach if error
condition.
> }
>
> + if (error) {
> + if (sbi->options.quiet)
> + error = 0;
> + goto out;
> + }
RE: [PATCH 5.7.y 0/5] exfat stable patches for 5.7.y
> Hi Namjae, Hi Sasha, > > On Thu, Jul 02, 2020 at 08:20:19AM +0900, Namjae Jeon wrote: > >Could you please push exfat stable patches into 5.7.y kernel tree ? > > I've queued them up, however it would be much easier if for commits that > don't require any > modification to allow backporting you would just provide the commit ids in > Linus's tree rather than > the patches themselves. > > I do see that you had to modify this one: > > >Sungjong Seo (1): > > exfat: flush dirty metadata in fsync > > In which case, a header in the commit message indicating the upstream commit > id would be appriciated. > Something like this: Okay, I'll do that next time! Thank you! > > [ Upstream commit 5267456e953fd8c5abd8e278b1cc6a9f9027ac0a ] > > Thank you! > > -- > Thanks, > Sasha
[PATCH 5.7.y 2/5] exfat: add missing brelse() calls on error paths
From: Dan Carpenter
If the second exfat_get_dentry() call fails then we need to release
"old_bh" before returning. There is a similar bug in exfat_move_file().
Fixes: 5f2aa075070c ("exfat: add inode operations")
Cc: sta...@vger.kernel.org # v5.7
Reported-by: Markus Elfring
Signed-off-by: Dan Carpenter
Signed-off-by: Namjae Jeon
---
fs/exfat/namei.c | 12 ++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/fs/exfat/namei.c b/fs/exfat/namei.c
index a2659a8a68a1..3bf1dbadab69 100644
--- a/fs/exfat/namei.c
+++ b/fs/exfat/namei.c
@@ -1089,10 +1089,14 @@ static int exfat_rename_file(struct inode *inode,
struct exfat_chain *p_dir,
epold = exfat_get_dentry(sb, p_dir, oldentry + 1, _bh,
_old);
+ if (!epold)
+ return -EIO;
epnew = exfat_get_dentry(sb, p_dir, newentry + 1, _bh,
_new);
- if (!epold || !epnew)
+ if (!epnew) {
+ brelse(old_bh);
return -EIO;
+ }
memcpy(epnew, epold, DENTRY_SIZE);
exfat_update_bh(sb, new_bh, sync);
@@ -1173,10 +1177,14 @@ static int exfat_move_file(struct inode *inode, struct
exfat_chain *p_olddir,
epmov = exfat_get_dentry(sb, p_olddir, oldentry + 1, _bh,
_mov);
+ if (!epmov)
+ return -EIO;
epnew = exfat_get_dentry(sb, p_newdir, newentry + 1, _bh,
_new);
- if (!epmov || !epnew)
+ if (!epnew) {
+ brelse(mov_bh);
return -EIO;
+ }
memcpy(epnew, epmov, DENTRY_SIZE);
exfat_update_bh(sb, new_bh, IS_DIRSYNC(inode));
--
2.17.1
[PATCH 5.7.y 0/5] exfat stable patches for 5.7.y
Could you please push exfat stable patches into 5.7.y kernel tree ? Thanks! Dan Carpenter (1): exfat: add missing brelse() calls on error paths Hyeongseok.Kim (1): exfat: Set the unused characters of FileName field to the value h Hyunchul Lee (1): exfat: call sync_filesystem for read-only remount Namjae Jeon (1): exfat: move setting VOL_DIRTY over exfat_remove_entries() Sungjong Seo (1): exfat: flush dirty metadata in fsync fs/exfat/dir.c | 12 +++- fs/exfat/exfat_fs.h | 1 + fs/exfat/file.c | 19 ++- fs/exfat/namei.c| 14 +++--- fs/exfat/super.c| 10 ++ 5 files changed, 47 insertions(+), 9 deletions(-) -- 2.17.1
[PATCH 5.7.y 5/5] exfat: flush dirty metadata in fsync
From: Sungjong Seo
generic_file_fsync() exfat used could not guarantee the consistency of
a file because it has flushed not dirty metadata but only dirty data pages
for a file.
Instead of that, use exfat_file_fsync() for files and directories so that
it guarantees to commit both the metadata and data pages for a file.
Fixes: 98d917047e8b ("exfat: add file operations")
Cc: sta...@vger.kernel.org # v5.7
Signed-off-by: Sungjong Seo
Signed-off-by: Namjae Jeon
---
fs/exfat/dir.c | 2 +-
fs/exfat/exfat_fs.h | 1 +
fs/exfat/file.c | 19 ++-
3 files changed, 20 insertions(+), 2 deletions(-)
diff --git a/fs/exfat/dir.c b/fs/exfat/dir.c
index 349ca0c282c2..6db302d76d4c 100644
--- a/fs/exfat/dir.c
+++ b/fs/exfat/dir.c
@@ -314,7 +314,7 @@ const struct file_operations exfat_dir_operations = {
.llseek = generic_file_llseek,
.read = generic_read_dir,
.iterate= exfat_iterate,
- .fsync = generic_file_fsync,
+ .fsync = exfat_file_fsync,
};
int exfat_alloc_new_dir(struct inode *inode, struct exfat_chain *clu)
diff --git a/fs/exfat/exfat_fs.h b/fs/exfat/exfat_fs.h
index d67fb8a6f770..d865050fa6cd 100644
--- a/fs/exfat/exfat_fs.h
+++ b/fs/exfat/exfat_fs.h
@@ -424,6 +424,7 @@ void exfat_truncate(struct inode *inode, loff_t size);
int exfat_setattr(struct dentry *dentry, struct iattr *attr);
int exfat_getattr(const struct path *path, struct kstat *stat,
unsigned int request_mask, unsigned int query_flags);
+int exfat_file_fsync(struct file *file, loff_t start, loff_t end, int
datasync);
/* namei.c */
extern const struct dentry_operations exfat_dentry_ops;
diff --git a/fs/exfat/file.c b/fs/exfat/file.c
index 5b4ddff18731..b93aa9e6cb16 100644
--- a/fs/exfat/file.c
+++ b/fs/exfat/file.c
@@ -6,6 +6,7 @@
#include
#include
#include
+#include
#include "exfat_raw.h"
#include "exfat_fs.h"
@@ -347,12 +348,28 @@ int exfat_setattr(struct dentry *dentry, struct iattr
*attr)
return error;
}
+int exfat_file_fsync(struct file *filp, loff_t start, loff_t end, int datasync)
+{
+ struct inode *inode = filp->f_mapping->host;
+ int err;
+
+ err = __generic_file_fsync(filp, start, end, datasync);
+ if (err)
+ return err;
+
+ err = sync_blockdev(inode->i_sb->s_bdev);
+ if (err)
+ return err;
+
+ return blkdev_issue_flush(inode->i_sb->s_bdev, GFP_KERNEL, NULL);
+}
+
const struct file_operations exfat_file_operations = {
.llseek = generic_file_llseek,
.read_iter = generic_file_read_iter,
.write_iter = generic_file_write_iter,
.mmap = generic_file_mmap,
- .fsync = generic_file_fsync,
+ .fsync = exfat_file_fsync,
.splice_read= generic_file_splice_read,
.splice_write = iter_file_splice_write,
};
--
2.17.1
[PATCH 5.7.y 4/5] exfat: move setting VOL_DIRTY over exfat_remove_entries()
Move setting VOL_DIRTY over exfat_remove_entries() to avoid unneeded
leaving VOL_DIRTY on -ENOTEMPTY.
Fixes: 5f2aa075070c ("exfat: add inode operations")
Cc: sta...@vger.kernel.org # v5.7
Reported-by: Tetsuhiro Kohada
Reviewed-by: Sungjong Seo
Signed-off-by: Namjae Jeon
---
fs/exfat/namei.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/exfat/namei.c b/fs/exfat/namei.c
index 3bf1dbadab69..2c9c78317721 100644
--- a/fs/exfat/namei.c
+++ b/fs/exfat/namei.c
@@ -984,7 +984,6 @@ static int exfat_rmdir(struct inode *dir, struct dentry
*dentry)
goto unlock;
}
- exfat_set_vol_flags(sb, VOL_DIRTY);
exfat_chain_set(_to_free, ei->start_clu,
EXFAT_B_TO_CLU_ROUND_UP(i_size_read(inode), sbi), ei->flags);
@@ -1012,6 +1011,7 @@ static int exfat_rmdir(struct inode *dir, struct dentry
*dentry)
num_entries++;
brelse(bh);
+ exfat_set_vol_flags(sb, VOL_DIRTY);
err = exfat_remove_entries(dir, , entry, 0, num_entries);
if (err) {
exfat_msg(sb, KERN_ERR,
--
2.17.1
[PATCH 5.7.y 1/5] exfat: Set the unused characters of FileName field to the value 0000h
From: "Hyeongseok.Kim"
Some fsck tool complain that padding part of the FileName field
is not set to the value h. So let's maintain filesystem cleaner,
as exfat's spec. recommendation.
Fixes: ca06197382bd ("exfat: add directory operations")
Cc: sta...@vger.kernel.org # v5.7
Signed-off-by: Hyeongseok.Kim
Reviewed-by: Sungjong Seo
Signed-off-by: Namjae Jeon
---
fs/exfat/dir.c | 10 ++
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/fs/exfat/dir.c b/fs/exfat/dir.c
index 4b91afb0f051..349ca0c282c2 100644
--- a/fs/exfat/dir.c
+++ b/fs/exfat/dir.c
@@ -430,10 +430,12 @@ static void exfat_init_name_entry(struct exfat_dentry *ep,
ep->dentry.name.flags = 0x0;
for (i = 0; i < EXFAT_FILE_NAME_LEN; i++) {
- ep->dentry.name.unicode_0_14[i] = cpu_to_le16(*uniname);
- if (*uniname == 0x0)
- break;
- uniname++;
+ if (*uniname != 0x0) {
+ ep->dentry.name.unicode_0_14[i] = cpu_to_le16(*uniname);
+ uniname++;
+ } else {
+ ep->dentry.name.unicode_0_14[i] = 0x0;
+ }
}
}
--
2.17.1
[PATCH 5.7.y 3/5] exfat: call sync_filesystem for read-only remount
From: Hyunchul Lee
We need to commit dirty metadata and pages to disk
before remounting exfat as read-only.
This fixes a failure in xfstests generic/452
generic/452 does the following:
cp something /
mount -o remount,ro
the /something is corrupted. because while
exfat is remounted as read-only, exfat doesn't
have a chance to commit metadata and
vfs invalidates page caches in a block device.
Fixes: 719c1e182916 ("exfat: add super block operations")
Cc: sta...@vger.kernel.org # v5.7
Signed-off-by: Hyunchul Lee
Acked-by: Sungjong Seo
Signed-off-by: Namjae Jeon
---
fs/exfat/super.c | 10 ++
1 file changed, 10 insertions(+)
diff --git a/fs/exfat/super.c b/fs/exfat/super.c
index c1b1ed306a48..e87980153398 100644
--- a/fs/exfat/super.c
+++ b/fs/exfat/super.c
@@ -637,10 +637,20 @@ static void exfat_free(struct fs_context *fc)
}
}
+static int exfat_reconfigure(struct fs_context *fc)
+{
+ fc->sb_flags |= SB_NODIRATIME;
+
+ /* volume flag will be updated in exfat_sync_fs */
+ sync_filesystem(fc->root->d_sb);
+ return 0;
+}
+
static const struct fs_context_operations exfat_context_ops = {
.parse_param= exfat_parse_param,
.get_tree = exfat_get_tree,
.free = exfat_free,
+ .reconfigure= exfat_reconfigure,
};
static int exfat_init_fs_context(struct fs_context *fc)
--
2.17.1
[GIT PULL] exfat fixes for 5.8-rc4
Hi Linus, This is exfat fixes pull request for v5.8-rc4. I add description of this pull request on below. Please pull exfat with following fixes. Thanks! The following changes since commit 9ebcfadb0610322ac537dd7aa5d9cbc2b2894c68: Linux 5.8-rc3 (2020-06-28 15:00:24 -0700) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/linkinjeon/exfat.git tags/exfat-for-5.8-rc4 for you to fetch changes up to 5267456e953fd8c5abd8e278b1cc6a9f9027ac0a: exfat: flush dirty metadata in fsync (2020-06-29 17:11:18 +0900) Description for this pull request: - Zero out unused characters of FileName field to avoid a complaint from some fsck tool. - Fix memory leak on error paths. - Fix unnecessary VOL_DIRTY set when calling rmdir on non-empty directory. - Call sync_filesystem() for read-only remount(Fix generic/452 test in xfstests) - Add own fsync() to flush dirty metadata. Dan Carpenter (1): exfat: add missing brelse() calls on error paths Hyeongseok.Kim (1): exfat: Set the unused characters of FileName field to the value h Hyunchul Lee (1): exfat: call sync_filesystem for read-only remount Namjae Jeon (1): exfat: move setting VOL_DIRTY over exfat_remove_entries() Sungjong Seo (1): exfat: flush dirty metadata in fsync fs/exfat/dir.c | 12 +++- fs/exfat/exfat_fs.h | 1 + fs/exfat/file.c | 19 ++- fs/exfat/namei.c| 14 +++--- fs/exfat/super.c| 10 ++ 5 files changed, 47 insertions(+), 9 deletions(-)
RE: [PATCH 1/2 v5] exfat: write multiple sectors at once
> Write multiple sectors at once when updating dir-entries. > Add exfat_update_bhs() for that. It wait for write completion once instead of > sector by sector. > It's only effective if sync enabled. > > Signed-off-by: Tetsuhiro Kohada Pushed your 2 patches to exfat #dev. Thanks!
RE: [PATCH] exfat: flush dirty metadata in fsync
> generic_file_fsync() exfat used could not guarantee the consistency of a file > because it has flushed > not dirty metadata but only dirty data pages for a file. > > Instead of that, use exfat_file_fsync() for files and directories so that it > guarantees to commit both > the metadata and data pages for a file. > > Signed-off-by: Sungjong Seo Pushed it into exfat #dev. Thanks!
Re: [PATCH 1/2 v4] exfat: write multiple sectors at once
2020-06-19 17:38 GMT+09:00, Tetsuhiro Kohada : > Write multiple sectors at once when updating dir-entries. > Add exfat_update_bhs() for that. It wait for write completion once > instead of sector by sector. > It's only effective if sync enabled. > > Reviewed-by: Christoph Hellwig He didn't give reviewed-by tag for this patch. You shouldn't add it at will. > Signed-off-by: Tetsuhiro Kohada
RE: [PATCH v3] exfat: remove EXFAT_SB_DIRTY flag
> > remove EXFAT_SB_DIRTY flag and related codes. > > > > This flag is set/reset in exfat_put_super()/exfat_sync_fs() to avoid > > sync_blockdev(). > > However ... > > - exfat_put_super(): > > Before calling this, the VFS has already called sync_filesystem(), so > > sync is never performed here. > > - exfat_sync_fs(): > > After calling this, the VFS calls sync_blockdev(), so, it is > > meaningless to check EXFAT_SB_DIRTY or to bypass sync_blockdev() here. > > Not only that, but in some cases can't clear VOL_DIRTY. > > ex: > > VOL_DIRTY is set when rmdir starts, but when non-empty-dir is > > detected, return error without setting EXFAT_SB_DIRTY. > > If performe 'sync' in this state, VOL_DIRTY will not be cleared. > > > > Since this patch does not resolve 'VOL_DIRTY in ENOTEMPTY' problem you > mentioned, it would be better > to remove the description above for that and to make new patch. > > > Remove the EXFAT_SB_DIRTY check to ensure synchronization. > > And, remove the code related to the flag. > > > > Signed-off-by: Tetsuhiro Kohada > > Reviewed-by: Sungjong Seo Applied. Thanks!
RE: [PATCH v2] exfat: call sync_filesystem for read-only remount
> > We need to commit dirty metadata and pages to disk before remounting > > exfat as read-only. > > > > This fixes a failure in xfstests generic/452 > > > > generic/452 does the following: > > cp something / > > mount -o remount,ro > > > > the /something is corrupted. because while exfat is remounted > > as read-only, exfat doesn't have a chance to commit metadata and vfs > > invalidates page caches in a block device. > > > > Signed-off-by: Hyunchul Lee > > Acked-by: Sungjong Seo Applied. Thanks!
RE: [PATCH v3] exfat: remove EXFAT_SB_DIRTY flag
> remove EXFAT_SB_DIRTY flag and related codes. > > This flag is set/reset in exfat_put_super()/exfat_sync_fs() to avoid > sync_blockdev(). > However ... > - exfat_put_super(): > Before calling this, the VFS has already called sync_filesystem(), so sync is > never performed here. > - exfat_sync_fs(): > After calling this, the VFS calls sync_blockdev(), so, it is meaningless to > check EXFAT_SB_DIRTY or to > bypass sync_blockdev() here. > Not only that, but in some cases can't clear VOL_DIRTY. > ex: > VOL_DIRTY is set when rmdir starts, but when non-empty-dir is detected, > return error without setting > EXFAT_SB_DIRTY. > If performe 'sync' in this state, VOL_DIRTY will not be cleared. There is still a problem if system reboot or device is unplugged when sync is not called yet. I will remove this mention in the patch.
RE: [PATCH 1/2] exfat: call sync_filesystem for read-only remount
> Hi Namjae,
>
> 2020년 6월 15일 (월) 오전 9:14, Namjae Jeon 님이 작성:
> >
> > Hi Hyunchul,
> > > We need to commit dirty metadata and pages to disk before remounting
> > > exfat as read-only.
> > >
> > > This fixes a failure in xfstests generic/452
> > Could you please elaborate more the reason why generic/452 in xfstests
> > failed ?
>
> xfstests generic/452 does the following.
> cp /bin/ls /
> mount -o remount,ro
>
> the /ls file is corrupted, because while exfat is remounted as
> read-only, exfat doesn't have a
> chance to commit metadata and vfs invalidates page caches in a block device.
Got it.
>
> I will put this explanation in a commit message.
Good.
>
> > >
> > > Signed-off-by: Hyunchul Lee
> > > ---
> > > fs/exfat/super.c | 19 +++
> > > 1 file changed, 19 insertions(+)
> > >
> > > diff --git a/fs/exfat/super.c b/fs/exfat/super.c index
> > > e650e65536f8..61c6cf240c19 100644
> > > --- a/fs/exfat/super.c
> > > +++ b/fs/exfat/super.c
> > > @@ -693,10 +693,29 @@ static void exfat_free(struct fs_context *fc)
> > > }
> > > }
> > >
> > > +static int exfat_reconfigure(struct fs_context *fc) {
> > > + struct super_block *sb = fc->root->d_sb;
> > > + int ret;
> > int ret = 0;
> > > + bool new_rdonly;
> > > +
> > > + new_rdonly = fc->sb_flags & SB_RDONLY;
> > > + if (new_rdonly != sb_rdonly(sb)) {
> > If you modify it like this, would not we need new_rdonly?
> > if (fc->sb_flags & SB_RDONLY && !sb_rdonly(sb))
> >
> This condition means that mount options are changed from "rw" to "ro", or
> "ro" to "rw".
>
> > > + if (new_rdonly) {
>
> And this condition means these options are changed from "rw" to "ro".
> It seems better to change two conditions to the one you suggested, or remove
> those. because
> sync_filesystem returns 0 when the filesystem is mounted as read-only.
The latter would be fine.
>
> > > + /* volume flag will be updated in exfat_sync_fs */
> > > + ret = sync_filesystem(sb);
> > > + if (ret < 0)
> > > + return ret;
> > I think that this ret check can be removed by using return ret; below ?
>
> Okay, I will apply this.
> Thank you for your comments!
Thanks for your patch!
>
>
> > > + }
> > > + }
> > > + return 0;
> > return ret;
> > > +}
> > > +
> > > static const struct fs_context_operations exfat_context_ops = {
> > > .parse_param= exfat_parse_param,
> > > .get_tree = exfat_get_tree,
> > > .free = exfat_free,
> > > + .reconfigure= exfat_reconfigure,
> > > };
> > >
> > > static int exfat_init_fs_context(struct fs_context *fc)
> > > --
> > > 2.17.1
> >
> >
RE: [PATCH 2/2] exfat: allow to change some mount options for remount
> Allow to change permission masks, allow_utime, errors. But ignore other
> options.
>
> Signed-off-by: Hyunchul Lee
> ---
> fs/exfat/super.c | 40 +---
> 1 file changed, 29 insertions(+), 11 deletions(-)
>
> diff --git a/fs/exfat/super.c b/fs/exfat/super.c index
> 61c6cf240c19..3c1d47289ba2 100644
> --- a/fs/exfat/super.c
> +++ b/fs/exfat/super.c
> @@ -696,9 +696,13 @@ static void exfat_free(struct fs_context *fc) static int
> exfat_reconfigure(struct fs_context *fc) {
> struct super_block *sb = fc->root->d_sb;
> + struct exfat_sb_info *sbi = EXFAT_SB(sb);
> + struct exfat_mount_options *new_opts;
> int ret;
> bool new_rdonly;
>
> + new_opts = &((struct exfat_sb_info *)fc->s_fs_info)->options;
> +
> new_rdonly = fc->sb_flags & SB_RDONLY;
> if (new_rdonly != sb_rdonly(sb)) {
> if (new_rdonly) {
> @@ -708,6 +712,12 @@ static int exfat_reconfigure(struct fs_context *fc)
> return ret;
> }
> }
> +
> + /* allow to change these options but ignore others */
> + sbi->options.fs_fmask = new_opts->fs_fmask;
> + sbi->options.fs_dmask = new_opts->fs_dmask;
> + sbi->options.allow_utime = new_opts->allow_utime;
> + sbi->options.errors = new_opts->errors;
Is there any reason why you allow a few options on remount ?
> return 0;
> }
>
> @@ -726,17 +736,25 @@ static int exfat_init_fs_context(struct fs_context *fc)
> if (!sbi)
> return -ENOMEM;
>
> - mutex_init(>s_lock);
> - ratelimit_state_init(>ratelimit, DEFAULT_RATELIMIT_INTERVAL,
> - DEFAULT_RATELIMIT_BURST);
> -
> - sbi->options.fs_uid = current_uid();
> - sbi->options.fs_gid = current_gid();
> - sbi->options.fs_fmask = current->fs->umask;
> - sbi->options.fs_dmask = current->fs->umask;
> - sbi->options.allow_utime = -1;
> - sbi->options.iocharset = exfat_default_iocharset;
> - sbi->options.errors = EXFAT_ERRORS_RO;
> + if (fc->root) {
> + /* reconfiguration */
> + memcpy(>options, _SB(fc->root->d_sb)->options,
> + sizeof(struct exfat_mount_options));
> + sbi->options.iocharset = exfat_default_iocharset;
> + } else {
> + mutex_init(>s_lock);
> + ratelimit_state_init(>ratelimit,
> + DEFAULT_RATELIMIT_INTERVAL,
> + DEFAULT_RATELIMIT_BURST);
> +
> + sbi->options.fs_uid = current_uid();
> + sbi->options.fs_gid = current_gid();
> + sbi->options.fs_fmask = current->fs->umask;
> + sbi->options.fs_dmask = current->fs->umask;
> + sbi->options.allow_utime = -1;
> + sbi->options.iocharset = exfat_default_iocharset;
> + sbi->options.errors = EXFAT_ERRORS_RO;
> + }
>
> fc->s_fs_info = sbi;
> fc->ops = _context_ops;
> --
> 2.17.1
RE: [PATCH 1/2] exfat: call sync_filesystem for read-only remount
Hi Hyunchul,
> We need to commit dirty metadata and pages to disk before remounting exfat as
> read-only.
>
> This fixes a failure in xfstests generic/452
Could you please elaborate more the reason why generic/452 in xfstests failed ?
>
> Signed-off-by: Hyunchul Lee
> ---
> fs/exfat/super.c | 19 +++
> 1 file changed, 19 insertions(+)
>
> diff --git a/fs/exfat/super.c b/fs/exfat/super.c index
> e650e65536f8..61c6cf240c19 100644
> --- a/fs/exfat/super.c
> +++ b/fs/exfat/super.c
> @@ -693,10 +693,29 @@ static void exfat_free(struct fs_context *fc)
> }
> }
>
> +static int exfat_reconfigure(struct fs_context *fc) {
> + struct super_block *sb = fc->root->d_sb;
> + int ret;
int ret = 0;
> + bool new_rdonly;
> +
> + new_rdonly = fc->sb_flags & SB_RDONLY;
> + if (new_rdonly != sb_rdonly(sb)) {
If you modify it like this, would not we need new_rdonly?
if (fc->sb_flags & SB_RDONLY && !sb_rdonly(sb))
> + if (new_rdonly) {
> + /* volume flag will be updated in exfat_sync_fs */
> + ret = sync_filesystem(sb);
> + if (ret < 0)
> + return ret;
I think that this ret check can be removed by using return ret; below ?
> + }
> + }
> + return 0;
return ret;
> +}
> +
> static const struct fs_context_operations exfat_context_ops = {
> .parse_param= exfat_parse_param,
> .get_tree = exfat_get_tree,
> .free = exfat_free,
> + .reconfigure= exfat_reconfigure,
> };
>
> static int exfat_init_fs_context(struct fs_context *fc)
> --
> 2.17.1
RE: [PATCH v2] exfat: add missing brelse() calls on error paths
> If the second exfat_get_dentry() call fails then we need to release "old_bh"
> before returning. There
> is a similar bug in exfat_move_file().
>
> Fixes: 5f2aa075070c ("exfat: add inode operations")
> Reported-by: Markus Elfring
> Signed-off-by: Dan Carpenter
Applied. Thanks!
RE: [PATCH 1/2 v2] exfat: write multiple sectors at once
> Write multiple sectors at once when updating dir-entries.
> Add exfat_update_bhs() for that. It wait for write completion once instead of
> sector by sector.
> It's only effective if sync enabled.
>
> Suggested-by: Namjae Jeon
> Signed-off-by: Tetsuhiro Kohada
> ---
> Changes in v2:
> - Split into 'write multiple sectors at once'
>and 'add error check when updating dir-entries'
>
> fs/exfat/dir.c | 12 +++-
> fs/exfat/exfat_fs.h | 1 +
> fs/exfat/misc.c | 19 +++
> 3 files changed, 27 insertions(+), 5 deletions(-)
>
> diff --git a/fs/exfat/dir.c b/fs/exfat/dir.c index de43534aa299..495884ccb352
> 100644
> --- a/fs/exfat/dir.c
> +++ b/fs/exfat/dir.c
> @@ -604,13 +604,15 @@ void exfat_update_dir_chksum_with_entry_set(struct
> exfat_entry_set_cache *es)
>
> void exfat_free_dentry_set(struct exfat_entry_set_cache *es, int sync) {
> - int i;
> + int i, err = 0;
>
> - for (i = 0; i < es->num_bh; i++) {
> - if (es->modified)
> - exfat_update_bh(es->sb, es->bh[i], sync);
> - brelse(es->bh[i]);
> + if (es->modified) {
> + set_bit(EXFAT_SB_DIRTY, _SB(es->sb)->s_state);
I pointed out that setting EXFAT_SB_DIRTY can be merged into exfat_update_bhs()
on previous thread.
Is it unnecessary?
> + err = exfat_update_bhs(es->bh, es->num_bh, sync);
> }
> +
> + for (i = 0; i < es->num_bh; i++)
> + err ? bforget(es->bh[i]):brelse(es->bh[i]);
> kfree(es);
> }
>
> diff --git a/fs/exfat/exfat_fs.h b/fs/exfat/exfat_fs.h index
> 595f3117f492..935954da2e54 100644
> --- a/fs/exfat/exfat_fs.h
> +++ b/fs/exfat/exfat_fs.h
> @@ -515,6 +515,7 @@ void exfat_set_entry_time(struct exfat_sb_info *sbi,
> struct timespec64 *ts,
> u16 exfat_calc_chksum16(void *data, int len, u16 chksum, int type);
> u32 exfat_calc_chksum32(void *data, int len, u32 chksum, int type); void
> exfat_update_bh(struct
> super_block *sb, struct buffer_head *bh, int sync);
> +int exfat_update_bhs(struct buffer_head **bhs, int nr_bhs, int sync);
> void exfat_chain_set(struct exfat_chain *ec, unsigned int dir,
> unsigned int size, unsigned char flags); void
> exfat_chain_dup(struct exfat_chain *dup,
> struct exfat_chain *ec); diff --git a/fs/exfat/misc.c b/fs/exfat/misc.c index
> 17d41f3d3709..dc34968e99d3 100644
> --- a/fs/exfat/misc.c
> +++ b/fs/exfat/misc.c
> @@ -173,6 +173,25 @@ void exfat_update_bh(struct super_block *sb, struct
> buffer_head *bh, int sync)
> sync_dirty_buffer(bh);
> }
>
> +int exfat_update_bhs(struct buffer_head **bhs, int nr_bhs, int sync) {
> + int i, err = 0;
> +
> + for (i = 0; i < nr_bhs; i++) {
> + set_buffer_uptodate(bhs[i]);
> + mark_buffer_dirty(bhs[i]);
> + if (sync)
> + write_dirty_buffer(bhs[i], 0);
> + }
> +
> + for (i = 0; i < nr_bhs && sync; i++) {
> + wait_on_buffer(bhs[i]);
> + if (!buffer_uptodate(bhs[i]))
> + err = -EIO;
> + }
> + return err;
> +}
> +
> void exfat_chain_set(struct exfat_chain *ec, unsigned int dir,
> unsigned int size, unsigned char flags) {
> --
> 2.25.1
[GIT PULL] exfat update for 5.8-rc1
Hi Linus, This is exfat update pull request for v5.8-rc1. I add description of this pull request on below. Please pull exfat with following ones. Thanks! The following changes since commit 3d77e6a8804abcc0504c904bd6e5cdf3a5cf8162: Linux 5.7 (2020-05-31 16:49:15 -0700) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/linkinjeon/exfat.git tags/exfat-for-5.8-rc1 for you to fetch changes up to fc961522ddbdf00254dd03b677627139cc1f68bc: exfat: Fix potential use after free in exfat_load_upcase_table() (2020-06-09 16:50:18 +0900) Description for this pull request: * Bug fixes - Fix memory leak on mount failure with iocharset= option. - Fix Incorrect update of stream entry. - Fix cluster range validation error. * Clean-up codes - Remove unused code and unneeded assignment. - Rename variables in exfat structure as specification. - Reorganize boot sector analysis code. - Simplify exfat_utf8_d_hash and exfat_utf8_d_cmp(). - Optimize exfat entry cache functions. - Improve wording of EXFAT_DEFAULT_IOCHARSET config option. * New Feature - Add boot region verification. Al Viro (1): exfat: fix memory leak in exfat_parse_param() Dan Carpenter (1): exfat: Fix potential use after free in exfat_load_upcase_table() Geert Uytterhoeven (1): exfat: Improve wording of EXFAT_DEFAULT_IOCHARSET config option Jason Yan (1): exfat: remove the assignment of 0 to bool variable Joe Perches (1): exfat: Use a more common logging style Namjae Jeon (2): exfat: remove unnecessary reassignment of p_uniname->name_len exfat: fix incorrect update of stream entry in __exfat_truncate() Pali Rohár (3): exfat: Simplify exfat_utf8_d_cmp() for code points above U+ exfat: Simplify exfat_utf8_d_hash() for code points above U+ exfat: Remove unused functions exfat_high_surrogate() and exfat_low_surrogate() Tetsuhiro Kohada (6): exfat: replace 'time_ms' with 'time_cs' exfat: optimize dir-cache exfat: redefine PBR as boot_sector exfat: separate the boot sector analysis exfat: add boot region verification exfat: standardize checksum calculation hyeongseok.kim (1): exfat: fix range validation error in alloc and free cluster fs/exfat/Kconfig | 7 +- fs/exfat/balloc.c| 8 +- fs/exfat/dir.c | 222 +-- fs/exfat/exfat_fs.h | 48 +- fs/exfat/exfat_raw.h | 85 +++-- fs/exfat/fatent.c| 17 ++-- fs/exfat/file.c | 25 +++-- fs/exfat/inode.c | 57 +-- fs/exfat/misc.c | 46 + fs/exfat/namei.c | 63 + fs/exfat/nls.c | 52 +++--- fs/exfat/super.c | 262 +++ 12 files changed, 423 insertions(+), 469 deletions(-)
RE: [PATCH 3/3] exfat: set EXFAT_SB_DIRTY and VOL_DIRTY at the same timing
> Thank you for your comment. > > > >> Can you split this patch into two? (Don't set VOL_DIRTY on > > >> -ENOTEMPTY and Setting EXFAT_SB_DIRTY is merged into > > >> exfat_set_vol_flag). I need to check the second one more. > > > > > > Can't do that. > > > > > > exfat_set_vol_flag() is called when rmdir processing begins. When > > > Not-empty is detected, VOL_DIRTY has already been written and synced > > > to the media. > > You can move it before calling exfat_remove_entries(). > > Can be moved, but that doesn't solve the problem. > It causes the similar problem as before. > > exfat_remove_entries() calls exfat_get_dentry(). > If exfat_get_dentry() fails, update bh and set SB_DIRTY will not be executed. > As a result, SB_DIRTY is not set and sync does not work. > Similar problems occur with other writing functions. > Similar problems occur when pre-write checks are added in the future. > > If you don't set VOL_DIRTY at the beginning, you should delay to set > VOL_DIRTY until update-bh & set > SB_DIRTY. > This avoids unnecessary changes to VOL_DIRTY/VOL_CLEAN. Right, That's what I am going to point out. > I think this method is smart, but it is difficult to decide when to set > VOL_CLEAN. > (I tried to implement it, but gave up) Okay, I'm a little busy now, but I'll give you feedback soon. Thanks for your work! > > > > By doing this, sync is guaranteed if VOL_DIRTY is set by calling > > > exfat_set_vol_flag. > > > > > > This change may still have problems, but it's little better than > > > before, I think. > > I need to check more if it is the best or there is more better way. > > I think the sync-problems still exist. > Let's improve little by little. :-) > > BR > --- > Kohada Tetsuhiro
RE: [PATCH] exfat: Fix pontential use after free in exfat_load_upcase_table()
> This code calls brelse(bh) and then dereferences "bh" on the next line
> resulting in a possible use
> after free. The brelse() should just be moved down a line.
>
> Fixes: b676fdbcf4c8 ("exfat: standardize checksum calculation")
> Signed-off-by: Dan Carpenter
Applied. Thanks!
> ---
> fs/exfat/nls.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/exfat/nls.c b/fs/exfat/nls.c index
> c1ec056954974..57b5a7a4d1f7a 100644
> --- a/fs/exfat/nls.c
> +++ b/fs/exfat/nls.c
> @@ -692,8 +692,8 @@ static int exfat_load_upcase_table(struct super_block *sb,
> index++;
> }
> }
> - brelse(bh);
> chksum = exfat_calc_chksum32(bh->b_data, i, chksum, CS_DEFAULT);
> + brelse(bh);
> }
>
> if (index >= 0x && utbl_checksum == chksum)
> --
> 2.26.2
Re: [PATCH 1/3] exfat: add error check when updating dir-entries
2020-06-04 17:44 GMT+09:00, Tetsuhiro Kohada :
Hi Tetsuhiro,
> Add error check when synchronously updating dir-entries.
> Furthermore, add exfat_update_bhs(). It wait for write completion once
> instead of sector by sector.
This patch can be split into two also ?
>
> Suggested-by: Sungjong Seo
> Signed-off-by: Tetsuhiro Kohada
> ---
> fs/exfat/dir.c | 15 +--
> fs/exfat/exfat_fs.h | 3 ++-
> fs/exfat/file.c | 5 -
> fs/exfat/inode.c| 8 +---
> fs/exfat/misc.c | 19 +++
> 5 files changed, 39 insertions(+), 11 deletions(-)
>
> diff --git a/fs/exfat/dir.c b/fs/exfat/dir.c
> index de43534aa299..3eb8386fb5f2 100644
> --- a/fs/exfat/dir.c
> +++ b/fs/exfat/dir.c
> @@ -602,16 +602,19 @@ void exfat_update_dir_chksum_with_entry_set(struct
> exfat_entry_set_cache *es)
> es->modified = true;
> }
>
> -void exfat_free_dentry_set(struct exfat_entry_set_cache *es, int sync)
> +int exfat_free_dentry_set(struct exfat_entry_set_cache *es, int sync)
> {
> - int i;
> + int i, err = 0;
>
> - for (i = 0; i < es->num_bh; i++) {
> - if (es->modified)
> - exfat_update_bh(es->sb, es->bh[i], sync);
> - brelse(es->bh[i]);
> + if (es->modified) {
> + set_bit(EXFAT_SB_DIRTY, _SB(es->sb)->s_state);
EXFAT_SB_DIRTY set can be merged into exfat_update_bhs() ?
> + err = exfat_update_bhs(es->bh, es->num_bh, sync);
> }
> +
> + for (i = 0; i < es->num_bh; i++)
> + err ? bforget(es->bh[i]):brelse(es->bh[i]);
> kfree(es);
> + return err;
> }
>
> static int exfat_walk_fat_chain(struct super_block *sb,
> diff --git a/fs/exfat/exfat_fs.h b/fs/exfat/exfat_fs.h
> index 595f3117f492..f4fa0e833486 100644
> --- a/fs/exfat/exfat_fs.h
> +++ b/fs/exfat/exfat_fs.h
> @@ -462,7 +462,7 @@ struct exfat_dentry *exfat_get_dentry_cached(struct
> exfat_entry_set_cache *es,
> int num);
> struct exfat_entry_set_cache *exfat_get_dentry_set(struct super_block *sb,
> struct exfat_chain *p_dir, int entry, unsigned int type);
> -void exfat_free_dentry_set(struct exfat_entry_set_cache *es, int sync);
> +int exfat_free_dentry_set(struct exfat_entry_set_cache *es, int sync);
> int exfat_count_dir_entries(struct super_block *sb, struct exfat_chain
> *p_dir);
>
> /* inode.c */
> @@ -515,6 +515,7 @@ void exfat_set_entry_time(struct exfat_sb_info *sbi,
> struct timespec64 *ts,
> u16 exfat_calc_chksum16(void *data, int len, u16 chksum, int type);
> u32 exfat_calc_chksum32(void *data, int len, u32 chksum, int type);
> void exfat_update_bh(struct super_block *sb, struct buffer_head *bh, int
> sync);
> +int exfat_update_bhs(struct buffer_head **bhs, int nr_bhs, int sync);
> void exfat_chain_set(struct exfat_chain *ec, unsigned int dir,
> unsigned int size, unsigned char flags);
> void exfat_chain_dup(struct exfat_chain *dup, struct exfat_chain *ec);
> diff --git a/fs/exfat/file.c b/fs/exfat/file.c
> index fce03f318787..37c8f04c1f8a 100644
> --- a/fs/exfat/file.c
> +++ b/fs/exfat/file.c
> @@ -153,6 +153,7 @@ int __exfat_truncate(struct inode *inode, loff_t
> new_size)
> struct timespec64 ts;
> struct exfat_dentry *ep, *ep2;
> struct exfat_entry_set_cache *es;
> + int err;
>
> es = exfat_get_dentry_set(sb, &(ei->dir), ei->entry,
> ES_ALL_ENTRIES);
> @@ -187,7 +188,9 @@ int __exfat_truncate(struct inode *inode, loff_t
> new_size)
> }
>
> exfat_update_dir_chksum_with_entry_set(es);
> - exfat_free_dentry_set(es, inode_needs_sync(inode));
> + err = exfat_free_dentry_set(es, inode_needs_sync(inode));
> + if (err)
> + return err;
> }
>
> /* cut off from the FAT chain */
> diff --git a/fs/exfat/inode.c b/fs/exfat/inode.c
> index ef7cf7a6d187..c0bfd1a586aa 100644
> --- a/fs/exfat/inode.c
> +++ b/fs/exfat/inode.c
> @@ -77,8 +77,7 @@ static int __exfat_write_inode(struct inode *inode, int
> sync)
> ep2->dentry.stream.size = ep2->dentry.stream.valid_size;
>
> exfat_update_dir_chksum_with_entry_set(es);
> - exfat_free_dentry_set(es, sync);
> - return 0;
> + return exfat_free_dentry_set(es, sync);
> }
>
> int exfat_write_inode(struct inode *inode, struct writeback_control *wbc)
> @@ -222,6 +221,7 @@ static int exfat_map_cluster(struct inode *inode,
> unsigned int clu_offset,
> if (ei->dir.dir != DIR_DELETED && modified) {
> struct exfat_dentry *ep;
> struct exfat_entry_set_cache *es;
> + int err;
>
> es = exfat_get_dentry_set(sb, &(ei->dir), ei->entry,
> ES_ALL_ENTRIES);
> @@ -240,7 +240,9 @@ static int exfat_map_cluster(struct inode *inode,
> unsigned int clu_offset,
>
Re: [PATCH 3/3] exfat: set EXFAT_SB_DIRTY and VOL_DIRTY at the same timing
2020-06-06 18:22 GMT+09:00, Tetsuhiro Kohada :
> On 2020/06/05 16:32, Namjae Jeon wrote:
>>> Set EXFAT_SB_DIRTY flag in exfat_put_super().
>>>
>>> In some cases, can't clear VOL_DIRTY with 'sync'.
>>> ex:
>>>
>>> VOL_DIRTY is set when rmdir starts, but when non-empty-dir is detected,
>>> return error without setting
>>> EXFAT_SB_DIRTY.
>>> If performe 'sync' in this state, VOL_DIRTY will not be cleared.
>> Good catch.
>>
>> Can you split this patch into two? (Don't set VOL_DIRTY on -ENOTEMPTY and
>> Setting EXFAT_SB_DIRTY is
>> merged into exfat_set_vol_flag). I need to check the second one more.
>
> Can't do that.
>
> exfat_set_vol_flag() is called when rmdir processing begins. When Not-empty
> is detected,
> VOL_DIRTY has already been written and synced to the media.
You can move it before calling exfat_remove_entries().
>
> This sequence is same as other write functions.
>
> set VOL_DIRTY (write to the media)
> preparation/state analysis
> update bh & set SB_DIRTY
> clear VOL_DIRTY (cached)
>
>
> SB_DIRTY is set when updating bh.
> However, in some cases SB_DIRTY is not set.
> If SB_DIRTY is not set, exfat_sync_fs() does nothing.
>
> I thought there was a problem with separating VOL_DIRTY and SB_DIRTY.
> So I investigated the timing when these are set. Attach the caller graph.
> The green box is a function that sets SB_DIRTY directly.
> The red box is a function that calls exfat_set_vol_flag() and sets
> VOL_DIRTY.
> VOL_DIRTY is set on all paths before setting SB_DIRTY.
>
> I think VOL_DIRTY and SB_DIRTY should be set at the same time.
> That way, It is not necessary to set SB_DIRTY when updating bh.
>
>
> set VOL_DIRTY (actually write on media), and set SB_DIRTY
> preparation/state analysis
> update data
> clear VOL_DIRTY (cached)
>
>
> By doing this, sync is guaranteed if VOL_DIRTY is set by calling
> exfat_set_vol_flag.
>
> This change may still have problems, but it's little better than before, I
> think.
I need to check more if it is the best or there is more better way.
Thanks!
>
> BR
> ---
> Tetsuhiro Kohada
>
>
>>
>> Thanks!
>>>
>>> Signed-off-by: Tetsuhiro Kohada
>>> ---
>>> fs/exfat/balloc.c | 4 ++--
>>> fs/exfat/dir.c | 18 --
>>> fs/exfat/exfat_fs.h | 2 +-
>>> fs/exfat/fatent.c | 6 +-
>>> fs/exfat/misc.c | 3 +--
>>> fs/exfat/namei.c| 12 ++--
>>> fs/exfat/super.c| 3 +++
>>> 7 files changed, 22 insertions(+), 26 deletions(-)
>>>
>>> diff --git a/fs/exfat/balloc.c b/fs/exfat/balloc.c index
>>> 4055eb00ea9b..a987919686c0 100644
>>> --- a/fs/exfat/balloc.c
>>> +++ b/fs/exfat/balloc.c
>>> @@ -158,7 +158,7 @@ int exfat_set_bitmap(struct inode *inode, unsigned
>>> int clu)
>>> b = BITMAP_OFFSET_BIT_IN_SECTOR(sb, ent_idx);
>>>
>>> set_bit_le(b, sbi->vol_amap[i]->b_data);
>>> - exfat_update_bh(sb, sbi->vol_amap[i], IS_DIRSYNC(inode));
>>> + exfat_update_bh(sbi->vol_amap[i], IS_DIRSYNC(inode));
>>> return 0;
>>> }
>>>
>>> @@ -180,7 +180,7 @@ void exfat_clear_bitmap(struct inode *inode, unsigned
>>> int clu)
>>> b = BITMAP_OFFSET_BIT_IN_SECTOR(sb, ent_idx);
>>>
>>> clear_bit_le(b, sbi->vol_amap[i]->b_data);
>>> - exfat_update_bh(sb, sbi->vol_amap[i], IS_DIRSYNC(inode));
>>> + exfat_update_bh(sbi->vol_amap[i], IS_DIRSYNC(inode));
>>>
>>> if (opts->discard) {
>>> int ret_discard;
>>> diff --git a/fs/exfat/dir.c b/fs/exfat/dir.c index
>>> 3eb8386fb5f2..96c9a817d928 100644
>>> --- a/fs/exfat/dir.c
>>> +++ b/fs/exfat/dir.c
>>> @@ -468,7 +468,7 @@ int exfat_init_dir_entry(struct inode *inode, struct
>>> exfat_chain *p_dir,
>>> >dentry.file.access_date,
>>> NULL);
>>>
>>> - exfat_update_bh(sb, bh, IS_DIRSYNC(inode));
>>> + exfat_update_bh(bh, IS_DIRSYNC(inode));
>>> brelse(bh);
>>>
>>> ep = exfat_get_dentry(sb, p_dir, entry + 1, , ); @@ -478,7
>>> +478,7 @@ int
>>> exfat_init_dir_entry(struct inode *inode, struct exfat_chain *p_dir,
>>> exfat_init_stream_entry(ep,
>>> (type == TYPE_FILE) ? ALLOC_FAT_CHAIN : ALLOC_NO_FAT_CHAIN,
>>> start_clu, size);
>>> - exfat
RE: [PATCH 3/3] exfat: set EXFAT_SB_DIRTY and VOL_DIRTY at the same timing
> Set EXFAT_SB_DIRTY flag in exfat_put_super().
>
> In some cases, can't clear VOL_DIRTY with 'sync'.
> ex:
>
> VOL_DIRTY is set when rmdir starts, but when non-empty-dir is detected,
> return error without setting
> EXFAT_SB_DIRTY.
> If performe 'sync' in this state, VOL_DIRTY will not be cleared.
Good catch.
Can you split this patch into two? (Don't set VOL_DIRTY on -ENOTEMPTY and
Setting EXFAT_SB_DIRTY is
merged into exfat_set_vol_flag). I need to check the second one more.
Thanks!
>
> Signed-off-by: Tetsuhiro Kohada
> ---
> fs/exfat/balloc.c | 4 ++--
> fs/exfat/dir.c | 18 --
> fs/exfat/exfat_fs.h | 2 +-
> fs/exfat/fatent.c | 6 +-
> fs/exfat/misc.c | 3 +--
> fs/exfat/namei.c| 12 ++--
> fs/exfat/super.c| 3 +++
> 7 files changed, 22 insertions(+), 26 deletions(-)
>
> diff --git a/fs/exfat/balloc.c b/fs/exfat/balloc.c index
> 4055eb00ea9b..a987919686c0 100644
> --- a/fs/exfat/balloc.c
> +++ b/fs/exfat/balloc.c
> @@ -158,7 +158,7 @@ int exfat_set_bitmap(struct inode *inode, unsigned int
> clu)
> b = BITMAP_OFFSET_BIT_IN_SECTOR(sb, ent_idx);
>
> set_bit_le(b, sbi->vol_amap[i]->b_data);
> - exfat_update_bh(sb, sbi->vol_amap[i], IS_DIRSYNC(inode));
> + exfat_update_bh(sbi->vol_amap[i], IS_DIRSYNC(inode));
> return 0;
> }
>
> @@ -180,7 +180,7 @@ void exfat_clear_bitmap(struct inode *inode, unsigned int
> clu)
> b = BITMAP_OFFSET_BIT_IN_SECTOR(sb, ent_idx);
>
> clear_bit_le(b, sbi->vol_amap[i]->b_data);
> - exfat_update_bh(sb, sbi->vol_amap[i], IS_DIRSYNC(inode));
> + exfat_update_bh(sbi->vol_amap[i], IS_DIRSYNC(inode));
>
> if (opts->discard) {
> int ret_discard;
> diff --git a/fs/exfat/dir.c b/fs/exfat/dir.c index 3eb8386fb5f2..96c9a817d928
> 100644
> --- a/fs/exfat/dir.c
> +++ b/fs/exfat/dir.c
> @@ -468,7 +468,7 @@ int exfat_init_dir_entry(struct inode *inode, struct
> exfat_chain *p_dir,
> >dentry.file.access_date,
> NULL);
>
> - exfat_update_bh(sb, bh, IS_DIRSYNC(inode));
> + exfat_update_bh(bh, IS_DIRSYNC(inode));
> brelse(bh);
>
> ep = exfat_get_dentry(sb, p_dir, entry + 1, , ); @@ -478,7
> +478,7 @@ int
> exfat_init_dir_entry(struct inode *inode, struct exfat_chain *p_dir,
> exfat_init_stream_entry(ep,
> (type == TYPE_FILE) ? ALLOC_FAT_CHAIN : ALLOC_NO_FAT_CHAIN,
> start_clu, size);
> - exfat_update_bh(sb, bh, IS_DIRSYNC(inode));
> + exfat_update_bh(bh, IS_DIRSYNC(inode));
> brelse(bh);
>
> return 0;
> @@ -514,7 +514,7 @@ int exfat_update_dir_chksum(struct inode *inode, struct
> exfat_chain *p_dir,
> }
>
> fep->dentry.file.checksum = cpu_to_le16(chksum);
> - exfat_update_bh(sb, fbh, IS_DIRSYNC(inode));
> + exfat_update_bh(fbh, IS_DIRSYNC(inode));
> release_fbh:
> brelse(fbh);
> return ret;
> @@ -536,7 +536,7 @@ int exfat_init_ext_entry(struct inode *inode, struct
> exfat_chain *p_dir,
> return -EIO;
>
> ep->dentry.file.num_ext = (unsigned char)(num_entries - 1);
> - exfat_update_bh(sb, bh, sync);
> + exfat_update_bh(bh, sync);
> brelse(bh);
>
> ep = exfat_get_dentry(sb, p_dir, entry + 1, , ); @@ -545,7
> +545,7 @@ int
> exfat_init_ext_entry(struct inode *inode, struct exfat_chain *p_dir,
>
> ep->dentry.stream.name_len = p_uniname->name_len;
> ep->dentry.stream.name_hash = cpu_to_le16(p_uniname->name_hash);
> - exfat_update_bh(sb, bh, sync);
> + exfat_update_bh(bh, sync);
> brelse(bh);
>
> for (i = EXFAT_FIRST_CLUSTER; i < num_entries; i++) { @@ -554,7 +554,7
> @@ int
> exfat_init_ext_entry(struct inode *inode, struct exfat_chain *p_dir,
> return -EIO;
>
> exfat_init_name_entry(ep, uniname);
> - exfat_update_bh(sb, bh, sync);
> + exfat_update_bh(bh, sync);
> brelse(bh);
> uniname += EXFAT_FILE_NAME_LEN;
> }
> @@ -578,7 +578,7 @@ int exfat_remove_entries(struct inode *inode, struct
> exfat_chain *p_dir,
> return -EIO;
>
> exfat_set_entry_type(ep, TYPE_DELETED);
> - exfat_update_bh(sb, bh, IS_DIRSYNC(inode));
> + exfat_update_bh(bh, IS_DIRSYNC(inode));
> brelse(bh);
> }
>
> @@ -606,10 +606,8 @@ int exfat_free_dentry_set(struct exfat_entry_set_cache
> *es, int sync) {
> int i, err = 0;
>
> - if (es->modified) {
> - set_bit(EXFAT_SB_DIRTY, _SB(es->sb)->s_state);
> + if (es->modified)
> err = exfat_update_bhs(es->bh, es->num_bh, sync);
> - }
>
> for (i = 0; i < es->num_bh; i++)
> err ? bforget(es->bh[i]):brelse(es->bh[i]);
> diff --git a/fs/exfat/exfat_fs.h b/fs/exfat/exfat_fs.h index
> f4fa0e833486..0e094d186612 100644
> --- a/fs/exfat/exfat_fs.h
> +++ b/fs/exfat/exfat_fs.h
> @@ -514,7
[PATCH] exfat: fix incorrect update of stream entry in __exfat_truncate()
At truncate, there is a problem of incorrect updating in the file entry
pointer instead of stream entry. This will cause the problem of
overwriting the time field of the file entry to new_size. Fix it to
update stream entry.
Fixes: 98d917047e8b ("exfat: add file operations")
Cc: sta...@vger.kernel.org # v5.7
Signed-off-by: Namjae Jeon
---
fs/exfat/file.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/fs/exfat/file.c b/fs/exfat/file.c
index 8e3f0eef45d7..fce03f318787 100644
--- a/fs/exfat/file.c
+++ b/fs/exfat/file.c
@@ -171,11 +171,11 @@ int __exfat_truncate(struct inode *inode, loff_t new_size)
/* File size should be zero if there is no cluster allocated */
if (ei->start_clu == EXFAT_EOF_CLUSTER) {
- ep->dentry.stream.valid_size = 0;
- ep->dentry.stream.size = 0;
+ ep2->dentry.stream.valid_size = 0;
+ ep2->dentry.stream.size = 0;
} else {
- ep->dentry.stream.valid_size = cpu_to_le64(new_size);
- ep->dentry.stream.size = ep->dentry.stream.valid_size;
+ ep2->dentry.stream.valid_size = cpu_to_le64(new_size);
+ ep2->dentry.stream.size = ep->dentry.stream.valid_size;
}
if (new_size == 0) {
--
2.17.1
RE: [PATCH] exfat: fix memory leak in exfat_parse_param()
> On Wed, Jun 03, 2020 at 10:29:57AM +0900, Namjae Jeon wrote: > > > exfat_free() should call exfat_free_iocharset() after stealing > > param->string instead of kstrdup in exfat_parse_param(). > > ITYM > extfat_free() should call exfat_free_iocharset(), to prevent a leak in > case we fail after > parsing iocharset= but before calling > get_tree_bdev() > > Additionally, there's no point copying param->string in > exfat_parse_param() - just steal it, leaving NULL in param->string. > That's independent from the leak or fix thereof - it's simply avoiding an > extra copy. Updated it in v2. Thanks!
[PATCH v2] exfat: fix memory leak in exfat_parse_param()
From: Al Viro butt3rflyh4ck reported memory leak found by syzkaller. A param->string held by exfat_mount_options. BUG: memory leak unreferenced object 0x88801972e090 (size 8): comm "syz-executor.2", pid 16298, jiffies 4295172466 (age 14.060s) hex dump (first 8 bytes): 6b 6f 69 38 2d 75 00 00 koi8-u.. backtrace: [<5bfe35d6>] kstrdup+0x36/0x70 mm/util.c:60 [<18ed3277>] exfat_parse_param+0x160/0x5e0 fs/exfat/super.c:276 [<7680462b>] vfs_parse_fs_param+0x2b4/0x610 fs/fs_context.c:147 [<97c027f2>] vfs_parse_fs_string+0xe6/0x150 fs/fs_context.c:191 [<371bf78f>] generic_parse_monolithic+0x16f/0x1f0 fs/fs_context.c:231 [<5ce5eb1b>] do_new_mount fs/namespace.c:2812 [inline] [<5ce5eb1b>] do_mount+0x12bb/0x1b30 fs/namespace.c:3141 [] __do_sys_mount fs/namespace.c:3350 [inline] [ ] __se_sys_mount fs/namespace.c:3327 [inline] [ ] __x64_sys_mount+0x18f/0x230 fs/namespace.c:3327 [<3b024e98>] do_syscall_64+0xf6/0x7d0 arch/x86/entry/common.c:295 [ ] entry_SYSCALL_64_after_hwframe+0x49/0xb3 exfat_free() should call exfat_free_iocharset(), to prevent a leak in case we fail after parsing iocharset= but before calling get_tree_bdev(). Additionally, there's no point copying param->string in exfat_parse_param() - just steal it, leaving NULL in param->string. That's independent from the leak or fix thereof - it's simply avoiding an extra copy. Fixes: 719c1e182916 ("exfat: add super block operations") Cc: sta...@vger.kernel.org # v5.7 Reported-by: butt3rflyh4ck Signed-off-by: Al Viro --- v2: - update patch description in more detail. fs/exfat/super.c | 12 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/fs/exfat/super.c b/fs/exfat/super.c index 405717e4e3ea..e650e65536f8 100644 --- a/fs/exfat/super.c +++ b/fs/exfat/super.c @@ -273,9 +273,8 @@ static int exfat_parse_param(struct fs_context *fc, struct fs_parameter *param) break; case Opt_charset: exfat_free_iocharset(sbi); - opts->iocharset = kstrdup(param->string, GFP_KERNEL); - if (!opts->iocharset) - return -ENOMEM; + opts->iocharset = param->string; + param->string = NULL; break; case Opt_errors: opts->errors = result.uint_32; @@ -686,7 +685,12 @@ static int exfat_get_tree(struct fs_context *fc) static void exfat_free(struct fs_context *fc) { - kfree(fc->s_fs_info); + struct exfat_sb_info *sbi = fc->s_fs_info; + + if (sbi) { + exfat_free_iocharset(sbi); + kfree(sbi); + } } static const struct fs_context_operations exfat_context_ops = { -- 2.17.1
[PATCH] exfat: fix memory leak in exfat_parse_param()
From: Al Viro butt3rflyh4ck reported memory leak found by syzkaller. A param->string held by exfat_mount_options. BUG: memory leak unreferenced object 0x88801972e090 (size 8): comm "syz-executor.2", pid 16298, jiffies 4295172466 (age 14.060s) hex dump (first 8 bytes): 6b 6f 69 38 2d 75 00 00 koi8-u.. backtrace: [<5bfe35d6>] kstrdup+0x36/0x70 mm/util.c:60 [<18ed3277>] exfat_parse_param+0x160/0x5e0 fs/exfat/super.c:276 [<7680462b>] vfs_parse_fs_param+0x2b4/0x610 fs/fs_context.c:147 [<97c027f2>] vfs_parse_fs_string+0xe6/0x150 fs/fs_context.c:191 [<371bf78f>] generic_parse_monolithic+0x16f/0x1f0 fs/fs_context.c:231 [<5ce5eb1b>] do_new_mount fs/namespace.c:2812 [inline] [<5ce5eb1b>] do_mount+0x12bb/0x1b30 fs/namespace.c:3141 [] __do_sys_mount fs/namespace.c:3350 [inline] [ ] __se_sys_mount fs/namespace.c:3327 [inline] [ ] __x64_sys_mount+0x18f/0x230 fs/namespace.c:3327 [<3b024e98>] do_syscall_64+0xf6/0x7d0 arch/x86/entry/common.c:295 [ ] entry_SYSCALL_64_after_hwframe+0x49/0xb3 exfat_free() should call exfat_free_iocharset() after stealing param->string instead of kstrdup in exfat_parse_param(). Fixes: 719c1e182916 ("exfat: add super block operations") Cc: sta...@vger.kernel.org # v5.7 Reported-by: butt3rflyh4ck Signed-off-by: Al Viro --- fs/exfat/super.c | 12 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/fs/exfat/super.c b/fs/exfat/super.c index 405717e4e3ea..e650e65536f8 100644 --- a/fs/exfat/super.c +++ b/fs/exfat/super.c @@ -273,9 +273,8 @@ static int exfat_parse_param(struct fs_context *fc, struct fs_parameter *param) break; case Opt_charset: exfat_free_iocharset(sbi); - opts->iocharset = kstrdup(param->string, GFP_KERNEL); - if (!opts->iocharset) - return -ENOMEM; + opts->iocharset = param->string; + param->string = NULL; break; case Opt_errors: opts->errors = result.uint_32; @@ -686,7 +685,12 @@ static int exfat_get_tree(struct fs_context *fc) static void exfat_free(struct fs_context *fc) { - kfree(fc->s_fs_info); + struct exfat_sb_info *sbi = fc->s_fs_info; + + if (sbi) { + exfat_free_iocharset(sbi); + kfree(sbi); + } } static const struct fs_context_operations exfat_context_ops = { -- 2.17.1
RE: memory leak in exfat_parse_param
> On Tue, Jun 02, 2020 at 01:03:05PM +0800, butt3rflyh4ck wrote:
> > I report a bug (in linux-5.7.0-rc7) found by syzkaller.
> >
> > kernel config:
> > https://protect2.fireeye.com/url?k=f3a88a7d-ae6446d8-f3a90132-0cc47a30
> > d446-6021a2fbdd1681a8=1=https%3A%2F%2Fgithub.com%2Fbutterflyhack%2
> > Fsyzkaller-fuzz%2Fblob%2Fmaster%2Fconfig-v5.7.0-rc7
> >
> > and can reproduce.
> >
> > A param->string held by exfat_mount_options.
>
> Humm...
>
> First of all, exfat_free() ought to call exfat_free_upcase_table().
> What's more, WTF bother with that kstrdup(), anyway? Just steal the string
> and be done with that...
Thanks for your patch. I will push it to exfat tree.
>
> Signed-off-by: Al Viro
> ---
> diff --git a/fs/exfat/super.c b/fs/exfat/super.c index
> 0565d5539d57..01cd7ed1614d 100644
> --- a/fs/exfat/super.c
> +++ b/fs/exfat/super.c
> @@ -259,9 +259,8 @@ static int exfat_parse_param(struct fs_context *fc,
> struct fs_parameter *param)
> break;
> case Opt_charset:
> exfat_free_iocharset(sbi);
> - opts->iocharset = kstrdup(param->string, GFP_KERNEL);
> - if (!opts->iocharset)
> - return -ENOMEM;
> + opts->iocharset = param->string;
> + param->string = NULL;
> break;
> case Opt_errors:
> opts->errors = result.uint_32;
> @@ -611,7 +610,10 @@ static int exfat_get_tree(struct fs_context *fc)
>
> static void exfat_free(struct fs_context *fc) {
> - kfree(fc->s_fs_info);
> + struct exfat_sb_info *sbi = fc->s_fs_info;
> +
> + exfat_free_iocharset(sbi);
> + kfree(sbi);
> }
>
> static const struct fs_context_operations exfat_context_ops = {
RE: [PATCH] exfat: optimize dir-cache
> > Optimize directory access based on exfat_entry_set_cache. > > - Hold bh instead of copied d-entry. > > - Modify bh->data directly instead of the copied d-entry. > > - Write back the retained bh instead of rescanning the d-entry-set. > > And > > - Remove unused cache related definitions. > > > > Signed-off-by: Tetsuhiro Kohada > > > > Reviewed-by: Sungjong Seo Applied your 5 patches. Thanks!
