Re: [PATCH] docs: filesystems: Fix a mundane typo
Bhaskar Chowdhury writes: > s/provisoned/provisioned/ > > Signed-off-by: Bhaskar Chowdhury Looks good. Acked-by: OGAWA Hirofumi > --- > Documentation/filesystems/vfat.rst | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/Documentation/filesystems/vfat.rst > b/Documentation/filesystems/vfat.rst > index e85d74e91295..760a4d83fdf9 100644 > --- a/Documentation/filesystems/vfat.rst > +++ b/Documentation/filesystems/vfat.rst > @@ -189,7 +189,7 @@ VFAT MOUNT OPTIONS > **discard** > If set, issues discard/TRIM commands to the block > device when blocks are freed. This is useful for SSD devices > - and sparse/thinly-provisoned LUNs. > + and sparse/thinly-provisioned LUNs. > > **nfs=stale_rw|nostale_ro** > Enable this only if you want to export the FAT filesystem > -- > 2.26.2 > -- OGAWA Hirofumi
Re: [PATCH] fs: fat: fix spelling typo of values
dingsen...@163.com writes: > From: dingsenjie > > vaules -> values > > Signed-off-by: dingsenjie Thanks. Acked-by: OGAWA Hirofumi > --- > fs/fat/fatent.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/fat/fatent.c b/fs/fat/fatent.c > index f7e3304..860e884 100644 > --- a/fs/fat/fatent.c > +++ b/fs/fat/fatent.c > @@ -771,7 +771,7 @@ int fat_trim_fs(struct inode *inode, struct fstrim_range > *range) > /* >* FAT data is organized as clusters, trim at the granulary of cluster. >* > - * fstrim_range is in byte, convert vaules to cluster index. > + * fstrim_range is in byte, convert values to cluster index. >* Treat sectors before data region as all used, not to trim them. >*/ > ent_start = max_t(u64, range->start>>sbi->cluster_bits, FAT_START_ENT); -- OGAWA Hirofumi
[PATCH v2] Fix zero_user_segments() with start > end
zero_user_segments() is used from __block_write_begin_int(), for
example like the following
zero_user_segments(page, 4096, 1024, 512, 918)
But new zero_user_segments() implements for HIGMEM + TRANSPARENT_HUGEPAGE
doesn't handle "start > end" case correctly, and hits BUG_ON(). (we
can fix __block_write_begin_int() instead though, it is the old and
multiple usage)
Also it calls kmap_atomic() unnecessary while start == end == 0.
Fixes: 0060ef3b4e6d ("mm: support THPs in zero_user_segments")
Cc:
Signed-off-by: OGAWA Hirofumi
---
mm/highmem.c | 17 -
1 file changed, 12 insertions(+), 5 deletions(-)
diff --git a/mm/highmem.c b/mm/highmem.c
index 874b732..86f2b94 100644
--- a/mm/highmem.c 2021-02-20 12:56:49.037165666 +0900
+++ b/mm/highmem.c 2021-02-20 22:03:08.369361223 +0900
@@ -368,20 +368,24 @@ void zero_user_segments(struct page *pag
BUG_ON(end1 > page_size(page) || end2 > page_size(page));
+ if (start1 >= end1)
+ start1 = end1 = 0;
+ if (start2 >= end2)
+ start2 = end2 = 0;
+
for (i = 0; i < compound_nr(page); i++) {
void *kaddr = NULL;
- if (start1 < PAGE_SIZE || start2 < PAGE_SIZE)
- kaddr = kmap_atomic(page + i);
-
if (start1 >= PAGE_SIZE) {
start1 -= PAGE_SIZE;
end1 -= PAGE_SIZE;
} else {
unsigned this_end = min_t(unsigned, end1, PAGE_SIZE);
- if (end1 > start1)
+ if (end1 > start1) {
+ kaddr = kmap_atomic(page + i);
memset(kaddr + start1, 0, this_end - start1);
+ }
end1 -= this_end;
start1 = 0;
}
@@ -392,8 +396,11 @@ void zero_user_segments(struct page *pag
} else {
unsigned this_end = min_t(unsigned, end2, PAGE_SIZE);
- if (end2 > start2)
+ if (end2 > start2) {
+ if (!kaddr)
+ kaddr = kmap_atomic(page + i);
memset(kaddr + start2, 0, this_end - start2);
+ }
end2 -= this_end;
start2 = 0;
}
_
--
OGAWA Hirofumi
Re: [PATCH] Fix zero_user_segments() with start > end
Matthew Wilcox writes:
> On Sat, Feb 27, 2021 at 01:11:35AM +0900, OGAWA Hirofumi wrote:
>> zero_user_segments() is used from __block_write_begin_int(), for
>> example like the following
>>
>> zero_user_segments(page, 4096, 1024, 512, 918)
>>
>> But new zero_user_segments() implements for HIGMEM + TRANSPARENT_HUGEPAGE
>> doesn't handle "start > end" case correctly, and hits BUG_ON(). (we
>> can fix __block_write_begin_int() instead though, it is the old and
>> multiple usage)
>
> Why don't we just take out the BUG_ON instead? The function doesn't
> actually do the wrong thing.
end1 is underflow with
if (start1 >= PAGE_SIZE) {
start1 -= PAGE_SIZE;
end1 -= PAGE_SIZE;
}
>> Also it calls kmap_atomic() unnecessary while start == end == 0.
>
> I'm OK with that. It always used to do that.
Old one is only one page, so it is always necessary if start1/end1 or
start2/end2 is valid range. But this one is multiple pages, so there are
completely unnecessary pages possibly.
>> Cc:
>> Signed-off-by: OGAWA Hirofumi
>
> Fixes: 0060ef3b4e6d ("mm: support THPs in zero_user_segments")
OK.
--
OGAWA Hirofumi
[PATCH] Fix zero_user_segments() with start > end
zero_user_segments() is used from __block_write_begin_int(), for
example like the following
zero_user_segments(page, 4096, 1024, 512, 918)
But new zero_user_segments() implements for HIGMEM + TRANSPARENT_HUGEPAGE
doesn't handle "start > end" case correctly, and hits BUG_ON(). (we
can fix __block_write_begin_int() instead though, it is the old and
multiple usage)
Also it calls kmap_atomic() unnecessary while start == end == 0.
Cc:
Signed-off-by: OGAWA Hirofumi
---
mm/highmem.c | 17 -
1 file changed, 12 insertions(+), 5 deletions(-)
diff --git a/mm/highmem.c b/mm/highmem.c
index 874b732..86f2b94 100644
--- a/mm/highmem.c 2021-02-20 12:56:49.037165666 +0900
+++ b/mm/highmem.c 2021-02-20 22:03:08.369361223 +0900
@@ -368,20 +368,24 @@ void zero_user_segments(struct page *pag
BUG_ON(end1 > page_size(page) || end2 > page_size(page));
+ if (start1 >= end1)
+ start1 = end1 = 0;
+ if (start2 >= end2)
+ start2 = end2 = 0;
+
for (i = 0; i < compound_nr(page); i++) {
void *kaddr = NULL;
- if (start1 < PAGE_SIZE || start2 < PAGE_SIZE)
- kaddr = kmap_atomic(page + i);
-
if (start1 >= PAGE_SIZE) {
start1 -= PAGE_SIZE;
end1 -= PAGE_SIZE;
} else {
unsigned this_end = min_t(unsigned, end1, PAGE_SIZE);
- if (end1 > start1)
+ if (end1 > start1) {
+ kaddr = kmap_atomic(page + i);
memset(kaddr + start1, 0, this_end - start1);
+ }
end1 -= this_end;
start1 = 0;
}
@@ -392,8 +396,11 @@ void zero_user_segments(struct page *pag
} else {
unsigned this_end = min_t(unsigned, end2, PAGE_SIZE);
- if (end2 > start2)
+ if (end2 > start2) {
+ if (!kaddr)
+ kaddr = kmap_atomic(page + i);
memset(kaddr + start2, 0, this_end - start2);
+ }
end2 -= this_end;
start2 = 0;
}
_
--
OGAWA Hirofumi
Re: [PATCH v6] fat: Add KUnit tests for checksums and timestamps
David Gow writes: > diff --git a/fs/fat/Kconfig b/fs/fat/Kconfig > index 66532a71e8fd..4e66f7e8defc 100644 > --- a/fs/fat/Kconfig > +++ b/fs/fat/Kconfig > @@ -115,3 +115,15 @@ config FAT_DEFAULT_UTF8 > Say Y if you use UTF-8 encoding for file names, N otherwise. > > See for more information. > + > +config FAT_KUNIT_TEST > + tristate "Unit Tests for FAT filesystems" if !KUNIT_ALL_TESTS > + depends on KUNIT && (MSDOS_FS || VFAT_FS) > + default KUNIT_ALL_TESTS > + help > + This builds the FAT KUnit tests > + > + For more information on KUnit and unit tests in general, please refer > + to the KUnit documentation in Documentation/dev-tools/kunit > + > + If unsure, say N Maybe, the following would be better? With this, it looks like kunit works whether depends on or select. Thanks. diff --git a/fs/fat/Kconfig b/fs/fat/Kconfig index ca31993..c1229d4 100644 --- a/fs/fat/Kconfig2020-09-02 20:48:54.967175266 +0900 +++ b/fs/fat/Kconfig2020-10-24 16:44:07.734324397 +0900 @@ -77,7 +77,7 @@ config VFAT_FS config FAT_DEFAULT_CODEPAGE int "Default codepage for FAT" - depends on MSDOS_FS || VFAT_FS + depends on FAT_FS default 437 help This option should be set to the codepage of your FAT filesystems. @@ -115,3 +115,15 @@ config FAT_DEFAULT_UTF8 Say Y if you use UTF-8 encoding for file names, N otherwise. See for more information. + +config FAT_KUNIT_TEST + tristate "Unit Tests for FAT filesystems" if !KUNIT_ALL_TESTS + depends on KUNIT && FAT_FS + default KUNIT_ALL_TESTS + help + This builds the FAT KUnit tests + + For more information on KUnit and unit tests in general, please refer + to the KUnit documentation in Documentation/dev-tools/kunit + + If unsure, say N _ -- OGAWA Hirofumi
Re: [PATCH v2] fat: Add KUnit tests for checksums and timestamps
David Gow writes: >> Hm, can this export only if FAT_KUNIT_TEST is builtin or module (maybe >> #if IS_ENABLED(...))? And #if will also be worked as the comment too. >> > > That's possible, but I'd prefer to export it unconditionally for two reasons: > 1. It'd make it possible to build the fat_test module without having > to rebuild the whole kernel/fat. > 2. It'd be consistent with fat_time_unix2fat(), which is exported for > use in vfat/msdos anyway. > > Neither of those are dealbreakers, though, so if you'd still prefer > this to be behind an ifdef, I'll change it. OK. If nobody complain, let's export. However, then, can you add the comment instead of ifdef to mark for kunit? Thanks. -- OGAWA Hirofumi
Re: [PATCH v2] fat: Add KUnit tests for checksums and timestamps
David Gow writes: > diff --git a/fs/fat/misc.c b/fs/fat/misc.c > index f1b2a1fc2a6a..445ad3542e74 100644 > --- a/fs/fat/misc.c > +++ b/fs/fat/misc.c > @@ -229,6 +229,7 @@ void fat_time_fat2unix(struct msdos_sb_info *sbi, struct > timespec64 *ts, > ts->tv_nsec = 0; > } > } > +EXPORT_SYMBOL_GPL(fat_time_fat2unix); Hm, can this export only if FAT_KUNIT_TEST is builtin or module (maybe #if IS_ENABLED(...))? And #if will also be worked as the comment too. > > /* Convert linear UNIX date to a FAT time/date pair. */ > void fat_time_unix2fat(struct msdos_sb_info *sbi, struct timespec64 *ts, -- OGAWA Hirofumi
Re: [PATCH] fat: Add KUnit tests for checksums and timestamps
David Gow writes:
> Add some basic sanity-check tests for the fat_checksum() function and
> the fat_time_unix2fat() and fat_time_fat2unix() functions. These unit
> tests verify these functions return correct output for a number of test
> inputs.
>
> These tests were inspored by -- and serve a similar purpose to -- the
> timestamp parsing KUnit tests in ext4[1].
>
> [1]:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/ext4/inode-test.c
>
> Signed-off-by: David Gow
Looks good, thanks.
Acked-by: OGAWA Hirofumi
> ---
> fs/fat/Kconfig| 13 +++
> fs/fat/Makefile | 2 +
> fs/fat/fat_test.c | 197 ++
> 3 files changed, 212 insertions(+)
> create mode 100644 fs/fat/fat_test.c
>
> diff --git a/fs/fat/Kconfig b/fs/fat/Kconfig
> index 66532a71e8fd..fdef03b79c69 100644
> --- a/fs/fat/Kconfig
> +++ b/fs/fat/Kconfig
> @@ -115,3 +115,16 @@ config FAT_DEFAULT_UTF8
> Say Y if you use UTF-8 encoding for file names, N otherwise.
>
> See for more information.
> +
> +config FAT_KUNIT_TEST
> + tristate "Unit Tests for FAT filesystems" if !KUNIT_ALL_TESTS
> + select FAT_FS
> + depends on KUNIT
> + default KUNIT_ALL_TESTS
> + help
> + This builds the FAT KUnit tests
> +
> + For more information on KUnit and unit tests in general, please refer
> + to the KUnit documentation in Documentation/dev-tools/kunit
> +
> + If unsure, say N
> diff --git a/fs/fat/Makefile b/fs/fat/Makefile
> index 70645ce2f7fc..2b034112690d 100644
> --- a/fs/fat/Makefile
> +++ b/fs/fat/Makefile
> @@ -10,3 +10,5 @@ obj-$(CONFIG_MSDOS_FS) += msdos.o
> fat-y := cache.o dir.o fatent.o file.o inode.o misc.o nfs.o
> vfat-y := namei_vfat.o
> msdos-y := namei_msdos.o
> +
> +obj-$(CONFIG_FAT_KUNIT_TEST) += fat_test.o
> diff --git a/fs/fat/fat_test.c b/fs/fat/fat_test.c
> new file mode 100644
> index ..c1b4348b9b3b
> --- /dev/null
> +++ b/fs/fat/fat_test.c
> @@ -0,0 +1,197 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * KUnit tests for FAT filesystems.
> + *
> + * Copyright (C) 2020 Google LLC.
> + * Author: David Gow
> + */
> +
> +#include
> +
> +#include "fat.h"
> +
> +static void fat_checksum_test(struct kunit *test)
> +{
> + /* With no extension. */
> + KUNIT_EXPECT_EQ(test, fat_checksum("VMLINUX"), 44);
> + /* With 3-letter extension. */
> + KUNIT_EXPECT_EQ(test, fat_checksum("README TXT"), 115);
> + /* With short (1-letter) extension. */
> + KUNIT_EXPECT_EQ(test, fat_checksum("ABCDEFGHA "), 98);
> +}
> +
> +
> +struct fat_timestamp_testcase {
> + const char *name;
> + struct timespec64 ts;
> + __le16 time;
> + __le16 date;
> + u8 cs;
> + int time_offset;
> +};
> +
> +const static struct fat_timestamp_testcase time_test_cases[] = {
> + {
> + .name = "Earliest possible UTC (1980-01-01 00:00:00)",
> + .ts = {.tv_sec = 315532800LL, .tv_nsec = 0L},
> + .time = 0,
> + .date = 33,
> + .cs = 0,
> + .time_offset = 0,
> + },
> + {
> + .name = "Latest possible UTC (2107-12-31 23:59:58)",
> + .ts = {.tv_sec = 4354819198LL, .tv_nsec = 0L},
> + .time = 49021,
> + .date = 65439,
> + .cs = 0,
> + .time_offset = 0,
> + },
> + {
> + .name = "Earliest possible (UTC-11) (== 1979-12-31 13:00:00
> UTC)",
> + .ts = {.tv_sec = 315493200LL, .tv_nsec = 0L},
> + .time = 0,
> + .date = 33,
> + .cs = 0,
> + .time_offset = 11 * 60,
> + },
> + {
> + .name = "Latest possible (UTC+11) (== 2108-01-01 10:59:58 UTC)",
> + .ts = {.tv_sec = 4354858798LL, .tv_nsec = 0L},
> + .time = 49021,
> + .date = 65439,
> + .cs = 0,
> + .time_offset = -11 * 60,
> + },
> + {
> + .name = "Leap Day / Year (1996-02-29 00:00:00)",
> + .ts = {.tv_sec = 825552000LL, .tv_nsec = 0L},
> + .time = 0,
> + .date = 8285,
> + .cs = 0,
> + .time_offset = 0,
> + },
> + {
> + .name = "Year 2000 is leap year (2000-02-29 00:00:00)",
> + .ts = {.tv_sec = 951782400LL, .tv_nsec = 0L},
> + .time = 0,
> + .date = 10333,
> + .cs = 0,
> +
Re: [PATCH] fat: Avoid oops when bdi->io_pages==0
Jens Axboe writes: > On 8/31/20 10:56 AM, Matthew Wilcox wrote: >> On Mon, Aug 31, 2020 at 10:39:26AM -0600, Jens Axboe wrote: >>> We really should ensure that ->io_pages is always set, imho, instead of >>> having to work-around it in other spots. >> >> Interestingly, there are only three places in the entire kernel which >> _use_ bdi->io_pages. FAT, Verity and the pagecache readahead code. >> >> Verity: >> unsigned long num_ra_pages = >> min_t(unsigned long, num_blocks_to_hash - i, >> inode->i_sb->s_bdi->io_pages); >> >> FAT: >> if (ra_pages > sb->s_bdi->io_pages) >> ra_pages = rounddown(ra_pages, sb->s_bdi->io_pages); >> >> Pagecache: >> max_pages = max_t(unsigned long, bdi->io_pages, ra->ra_pages); >> and >> if (req_size > max_pages && bdi->io_pages > max_pages) >> max_pages = min(req_size, bdi->io_pages); >> >> The funny thing is that all three are using it differently. Verity is >> taking io_pages to be the maximum amount to readahead. FAT is using >> it as the unit of readahead (round down to the previous multiple) and >> the pagecache uses it to limit reads that exceed the current per-file >> readahead limit (but allows per-file readahead to exceed io_pages, >> in which case it has no effect). >> >> So how should it be used? My inclination is to say that the pagecache >> is right, by virtue of being the most-used. > > When I added ->io_pages, it was for the page cache use case. The others > grew after that... FAT and pagecache usage would be similar or same purpose. The both is using io_pages as optimal IO size. In pagecache case, it uses io_pages if one request size is exceeding io_pages. In FAT case, there is perfect knowledge about future/total request size. So FAT divides request by io_pages, and adjust ra_pages with knowledge. I don't know about verity. Thanks. -- OGAWA Hirofumi
Re: [PATCH] fat: Avoid oops when bdi->io_pages==0
Jens Axboe writes: > On 8/31/20 10:37 AM, OGAWA Hirofumi wrote: >> Jens Axboe writes: >> >>> I don't think we should work-around this here. What device is this on? >>> Something like the below may help. >> >> The reported bug is from nvme stack, and the below patch (I submitted >> same patch to you) fixed the reported case though. But I didn't verify >> all possible path, so I'd liked to use safer side. >> >> If block layer can guarantee io_pages!=0 instead, and can apply to >> stable branch (5.8+). It would work too. > > We really should ensure that ->io_pages is always set, imho, instead of > having to work-around it in other spots. I think it is good too. However, the issue would be how to do it for stable branch. If you think that block layer patch is enough and submit to stable (5.8+) branch instead, I'm fine without fatfs patch. (Or removing workaround in fatfs with block layer patch later?) Thanks. -- OGAWA Hirofumi
Re: [PATCH] fat: Avoid oops when bdi->io_pages==0
Jens Axboe writes: > On Sat, Aug 29, 2020 at 7:08 PM OGAWA Hirofumi > wrote: >> >> On one system, there was bdi->io_pages==0. This seems to be the bug of >> a driver somewhere, and should fix it though. Anyway, it is better to >> avoid the divide-by-zero Oops. >> >> So this check it. >> >> Signed-off-by: OGAWA Hirofumi >> Cc: >> --- >> fs/fat/fatent.c |2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/fs/fat/fatent.c b/fs/fat/fatent.c >> index f7e3304..98a1c4f 100644 >> --- a/fs/fat/fatent.c 2020-08-30 06:52:47.251564566 +0900 >> +++ b/fs/fat/fatent.c 2020-08-30 06:54:05.838319213 +0900 >> @@ -660,7 +660,7 @@ static void fat_ra_init(struct super_blo >> if (fatent->entry >= ent_limit) >> return; >> >> - if (ra_pages > sb->s_bdi->io_pages) >> + if (sb->s_bdi->io_pages && ra_pages > sb->s_bdi->io_pages) >> ra_pages = rounddown(ra_pages, sb->s_bdi->io_pages); >> reada_blocks = ra_pages << (PAGE_SHIFT - sb->s_blocksize_bits + 1); > > I don't think we should work-around this here. What device is this on? > Something like the below may help. The reported bug is from nvme stack, and the below patch (I submitted same patch to you) fixed the reported case though. But I didn't verify all possible path, so I'd liked to use safer side. If block layer can guarantee io_pages!=0 instead, and can apply to stable branch (5.8+). It would work too. Thanks. > diff --git a/block/blk-core.c b/block/blk-core.c > index d9d632639bd1..10c08ac50697 100644 > --- a/block/blk-core.c > +++ b/block/blk-core.c > @@ -539,6 +539,7 @@ struct request_queue *blk_alloc_queue(int node_id) > goto fail_stats; > > q->backing_dev_info->ra_pages = VM_READAHEAD_PAGES; > + q->backing_dev_info->io_pages = VM_READAHEAD_PAGES; > q->backing_dev_info->capabilities = BDI_CAP_CGROUP_WRITEBACK; > q->node = node_id; -- OGAWA Hirofumi
Re: [PATCH] fat: Avoid oops when bdi->io_pages==0
Sasha Levin writes: > Hi > > [This is an automated email] > > This commit has been processed because it contains a -stable tag. > The stable tag indicates that it's relevant for the following trees: all > > The bot has tested the following trees: v5.8.5, v5.4.61, v4.19.142, > v4.14.195, v4.9.234, v4.4.234. > > v5.8.5: Build OK! [...] > > How should we proceed with this patch? Only 5.8.x has to apply this patch. Thanks. -- OGAWA Hirofumi
Re: [PATCH] fat: Avoid oops when bdi->io_pages==0
Matthew Wilcox writes: > On Sun, Aug 30, 2020 at 10:54:35AM +0900, OGAWA Hirofumi wrote: >> Matthew Wilcox writes: >> >> Hm, io_pages is limited by driver setting too, and io_pages can be lower >> than ra_pages, e.g. usb storage. >> >> Assuming ra_pages is user intent of readahead window. So if io_pages is >> lower than ra_pages, this try ra_pages to align of io_pages chunk, but >> not bigger than ra_pages. Because if block layer splits I/O requests to >> hard limit, then I/O is not optimal. >> >> So it is intent, I can be misunderstanding though. > > Looking at this some more, I'm not sure it makes sense to consult ->io_pages > at all. I see how it gets set to 0 -- the admin can write '1' to > /sys/block//queue/max_sectors_kb and that gets turned into 0 > in ->io_pages. if (max_sectors_kb > max_hw_sectors_kb || max_sectors_kb < page_kb) return -EINVAL; It should not set to 0 via /sys/.../max_sectors_kb. However the default of bdi->io_pages is 0. So it happened if a driver didn't initialized it. > But I'm not sure it makes any sense to respect that. Looking at > mm/readahead.c, all it does is limit the size of a read request which > exceeds the current readahead window. It's not used to limit the > readahead window itself. For example: > > unsigned long max_pages = ra->ra_pages; > ... > if (req_size > max_pages && bdi->io_pages > max_pages) > max_pages = min(req_size, bdi->io_pages); > > Setting io_pages below ra_pages has no effect. So maybe fat should also > disregard it? |---| requested blocks [before] ra_pages |===|===|===| io_pages |-|-|-| req |-|-|---|---| [after] ra_pages |=|=|=| io_pages |-|-|-| req |-|-----|---| This path is known the large sequential read there. Well, anyway, this intent is to use [after] as 3 req, instead of [before] as 4 req. Thanks. -- OGAWA Hirofumi
Re: [PATCH] fat: Avoid oops when bdi->io_pages==0
Matthew Wilcox writes: > On Sun, Aug 30, 2020 at 09:59:41AM +0900, OGAWA Hirofumi wrote: >> On one system, there was bdi->io_pages==0. This seems to be the bug of >> a driver somewhere, and should fix it though. Anyway, it is better to >> avoid the divide-by-zero Oops. >> >> So this check it. >> >> Signed-off-by: OGAWA Hirofumi >> Cc: >> --- >> fs/fat/fatent.c |2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/fs/fat/fatent.c b/fs/fat/fatent.c >> index f7e3304..98a1c4f 100644 >> --- a/fs/fat/fatent.c2020-08-30 06:52:47.251564566 +0900 >> +++ b/fs/fat/fatent.c2020-08-30 06:54:05.838319213 +0900 >> @@ -660,7 +660,7 @@ static void fat_ra_init(struct super_blo >> if (fatent->entry >= ent_limit) >> return; >> >> -if (ra_pages > sb->s_bdi->io_pages) >> +if (sb->s_bdi->io_pages && ra_pages > sb->s_bdi->io_pages) >> ra_pages = rounddown(ra_pages, sb->s_bdi->io_pages); > > Wait, rounddown? ->io_pages is supposed to be the maximum number of > pages to readahead. Shouldn't this be max() instead of rounddown()? Hm, io_pages is limited by driver setting too, and io_pages can be lower than ra_pages, e.g. usb storage. Assuming ra_pages is user intent of readahead window. So if io_pages is lower than ra_pages, this try ra_pages to align of io_pages chunk, but not bigger than ra_pages. Because if block layer splits I/O requests to hard limit, then I/O is not optimal. So it is intent, I can be misunderstanding though. Thanks. -- OGAWA Hirofumi
[PATCH] fat: Avoid oops when bdi->io_pages==0
On one system, there was bdi->io_pages==0. This seems to be the bug of a driver somewhere, and should fix it though. Anyway, it is better to avoid the divide-by-zero Oops. So this check it. Signed-off-by: OGAWA Hirofumi Cc: --- fs/fat/fatent.c |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/fat/fatent.c b/fs/fat/fatent.c index f7e3304..98a1c4f 100644 --- a/fs/fat/fatent.c 2020-08-30 06:52:47.251564566 +0900 +++ b/fs/fat/fatent.c 2020-08-30 06:54:05.838319213 +0900 @@ -660,7 +660,7 @@ static void fat_ra_init(struct super_blo if (fatent->entry >= ent_limit) return; - if (ra_pages > sb->s_bdi->io_pages) + if (sb->s_bdi->io_pages && ra_pages > sb->s_bdi->io_pages) ra_pages = rounddown(ra_pages, sb->s_bdi->io_pages); reada_blocks = ra_pages << (PAGE_SHIFT - sb->s_blocksize_bits + 1); _ -- OGAWA Hirofumi
[PATCH] block: Set default value to bdi->io_pages instead of zero
This may not enough to guarantee ->io_pages is not zero though, instead of leaving ->io_pages as zero, this initializing ->io_pages to sane value. (maybe some part of NVMe driver seems to be) Signed-off-by: OGAWA Hirofumi --- block/blk-core.c |1 + 1 file changed, 1 insertion(+) diff --git a/block/blk-core.c b/block/blk-core.c index 03252af..619a3dc 100644 --- a/block/blk-core.c 2020-08-30 06:47:21.221530450 +0900 +++ b/block/blk-core.c 2020-08-30 06:48:05.805875540 +0900 @@ -526,6 +526,7 @@ struct request_queue *__blk_alloc_queue( goto fail_stats; q->backing_dev_info->ra_pages = VM_READAHEAD_PAGES; + q->backing_dev_info->io_pages = VM_READAHEAD_PAGES; q->backing_dev_info->capabilities = BDI_CAP_CGROUP_WRITEBACK; q->node = node_id; _ -- OGAWA Hirofumi
[PATCH] fat: Fix fat_ra_init() for data clusters == 0
If data clusters == 0, fat_ra_init() calls the ->ent_blocknr() for the cluster beyond ->max_clusters. This checks the limit before initialization to suppress the warning. Reported-by: syzbot+756199124937b31a9...@syzkaller.appspotmail.com Signed-off-by: OGAWA Hirofumi --- fs/fat/fatent.c |3 +++ 1 file changed, 3 insertions(+) diff --git a/fs/fat/fatent.c b/fs/fat/fatent.c index bbfe18c..f7e3304 100644 --- a/fs/fat/fatent.c 2020-07-11 19:58:41.903092419 +0900 +++ b/fs/fat/fatent.c 2020-07-11 19:58:51.545948758 +0900 @@ -657,6 +657,9 @@ static void fat_ra_init(struct super_blo unsigned long ra_pages = sb->s_bdi->ra_pages; unsigned int reada_blocks; + if (fatent->entry >= ent_limit) + return; + if (ra_pages > sb->s_bdi->io_pages) ra_pages = rounddown(ra_pages, sb->s_bdi->io_pages); reada_blocks = ra_pages << (PAGE_SHIFT - sb->s_blocksize_bits + 1); _
Re: [PATCH] VFAT/FAT/MSDOS FILESYSTEM: Replace HTTP links with HTTPS ones
"Alexander A. Klimov" writes: > Rationale: > Reduces attack surface on kernel devs opening the links for MITM > as HTTPS traffic is much harder to manipulate. > > Deterministic algorithm: > For each file: > If not .svg: > For each line: > If doesn't contain `\bxmlns\b`: > For each link, `\bhttp://[^# \t\r\n]*(?:\w|/)`: > If neither `\bgnu\.org/license`, nor `\bmozilla\.org/MPL\b`: > If both the HTTP and HTTPS versions > return 200 OK and serve the same content: > Replace HTTP with HTTPS. > > Signed-off-by: Alexander A. Klimov Acked-by: OGAWA Hirofumi > --- > Continuing my work started at 93431e0607e5. > See also: git log --oneline '--author=Alexander A. Klimov > ' v5.7..master > (Actually letting a shell for loop submit all this stuff for me.) > > If there are any URLs to be removed completely or at least not HTTPSified: > Just clearly say so and I'll *undo my change*. > See also: https://lkml.org/lkml/2020/6/27/64 > > If there are any valid, but yet not changed URLs: > See: https://lkml.org/lkml/2020/6/26/837 > > If you apply the patch, please let me know. > > > fs/fat/Kconfig | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/fat/Kconfig b/fs/fat/Kconfig > index ca31993dcb47..66532a71e8fd 100644 > --- a/fs/fat/Kconfig > +++ b/fs/fat/Kconfig > @@ -41,7 +41,7 @@ config MSDOS_FS > they are compressed; to access compressed MSDOS partitions under > Linux, you can either use the DOS emulator DOSEMU, described in the > DOSEMU-HOWTO, available from > - <http://www.tldp.org/docs.html#howto>, or try dmsdosfs in > + <https://www.tldp.org/docs.html#howto>, or try dmsdosfs in > <ftp://ibiblio.org/pub/Linux/system/filesystems/dosfs/>. If you > intend to use dosemu with a non-compressed MSDOS partition, say Y > here) and MSDOS floppies. This means that file access becomes -- OGAWA Hirofumi
Re: [PATCH] fs: fat: add check for dir size in fat_calc_dir_size
Anupam Aggarwal writes: >>Anyway, fsck would be main way. And on other hand, if we want to add >>mitigation for corruption, we would have to see much more details of >>this corruption. Can you put somewhere to access the corrupted image >>(need the only metadata) to reproduce? > > Sorry, uploading of any file not allowed from within. > So, metadata image is not possible to be shared via. upload. > Can try to arrange few more logs via. fsck. Then, can you dump the invalid directory entries in corrupted image, and check exactly why recursive traverse (ls -lR) never end? We need to know the root cause to fix, e.g. this directory entry has loop, etc. >>What happens if you recursively traversed directories on Windows? This >>issue happens on Windows too? > > After connecting USB to windows 10, when corrupted dir(\CorruptedDIR) > is browsed, it shows 2623 number of files and directories, without > delay. Name and timestamps of those file/directories are garbage > values. Sounds like filtered the invalid names. > Further if we browse these sub-directories and open files of corrupted > dir(\CorruptedDIR) following popups are coming on Windows 10: > 1. The filename, directory name, or volume label syntax is incorrect > 2. Specified path does not exist. Check the path and try again > > So issue of un-ending browsing(ls -lR) of corrupted USB is not coming > on windows 10, it lists limited number of files/directories, of > corrupted dir(\CorruptedDIR) without delay. It may had the luck, loop was filtered by invalid names. Well, not sure. -- OGAWA Hirofumi
Re: [PATCH] fs: fat: add check for dir size in fat_calc_dir_size
Anupam Aggarwal writes: >>So what was the root cause of slowness on big directory? > > Problem happened on FAT32 formatted 32GB USB 3.0 pendrive, which has > 20GB of data, cluster size is 16KB It has one corrupted directory > whose size calculated by fat_calc_dir_size() is 1146896384 bytes > i.e. 1.06 GB. > > When directory traversal of corrupted directory starts, directory > entries looks to be corrupted and lookup fails for these directory > entries. Some directory entries name are having format abc/xyz, > following are the few observed directory entry names: [...] > During search for single name in fat_search_long() function, whole > corrupted directory of size 1.06GB is traversed, which takes around > 230 to 240 secs, which finally ends up with returning ENOENT. > > Now multiple lookups in corrupted directory makes “ls -lR” > never-ending e.g. in overnite test of running “ls –lR” on USB having > corrupted directory, around 200 such lookups in corrupted directory > took 14hrs and still ”ls –lR” is running. Sounds like totally corrupted FAT image, and the directory may have the non-simple loop (e.g. there is hardlink of directory). If so, I'm not sure if we can detect without heavyweight check. Well, although user should run fsck before mount. However, if fs can detect and stop early, it would be better. BTW, if you run fsck, the corrupted directories and issue are gone at least? Anyway, fsck would be main way. And on other hand, if we want to add mitigation for corruption, we would have to see much more details of this corruption. Can you put somewhere to access the corrupted image (need the only metadata) to reproduce? > Total number of directory entries in corrupted directory of size > 1146896384 bytes = 1146896384/32 = 35840512, so lookup for 35840512 > looks very exhaustive, therefore we have put size check of directory > in fat_calc_dir_size() and prevented the directory traversal by > returning -EIO. > > While browsing corrupted directory(\CorruptedDIR) on Windows 10 PC, > 2623 directory entries were listed and timestamps were wrong What happens if you recursively traversed directories on Windows? This issue happens on Windows too? Thanks. -- OGAWA Hirofumi
Re: (2) [PATCH] fs: fat: add check for dir size in fat_calc_dir_size
AMIT SAHRAWAT writes: > There are many implementation that doesn't follow the spec strictly. And > when I tested in past, Windows also allowed to read the directory beyond > that limit. I can't recall though if there is in real case or just test > case though. >>> Thanks Ogawa, yes there are many implementations, preferably going around >>> with different variants. > But, using standard linux version on the systems and having such USB > connected on such systems is introducing issues(importantly because these > being used on Windows also by users). > I am not sure, if this is something which is new from Windows part. > But, surely extending the directory beyond limit is causing regression with > FAT usage on linux. regression from what? > It is making FAT filesystem related storage virtually unresponsive for > minutes in these cases, > and importantly keep on putting pressure on memory due to increasing buffer > heads (already a known one with FAT fs). I'm confused. What happen actually? Now looks like you are saying the issue is extending size beyond limit. But previously it said the corruption. Are you saying "beyond that limit" is the fs corruption? I.e. did you met real directory corruption? or you are trying to limit because slowness on big directory? > So if there is no strong reason to apply the limit, I don't think it is > good to limit it. >>> The reason for us to share this is because of the unresponsive behaviour >>> observed with FAT fs on our systems. > This is not a new issue, we have been observing this for quite sometime (may > be around 1year+). > Finally, we got hold of disk which is making this 100% reproducible. > We thought of applying this to the mainline, as our FAT is aligned with main > kernel. So what was the root cause of slowness on big directory? Thanks. -- OGAWA Hirofumi
Re: [PATCH] fs: fat: add check for dir size in fat_calc_dir_size
Anupam Aggarwal writes:
> Max directory size of FAT filesystem is FAT_MAX_DIR_SIZE(2097152 bytes)
> It is possible that, due to corruption, directory size calculated in
> fat_calc_dir_size() can be greater than FAT_MAX_DIR_SIZE, i.e.
> can be in GBs, hence directory traversal can take long time.
> for example when command "ls -lR" is executed on corrupted FAT
> formatted USB, fat_search_long() function will lookup for a filename from
> position 0 till end of corrupted directory size, multiple such lookups
> will lead to long directory traversal
>
> Added sanity check for directory size fat_calc_dir_size(),
> and return EIO error, which will prevent lookup in corrupted directory
>
> Signed-off-by: Anupam Aggarwal
> Signed-off-by: Amit Sahrawat
There are many implementation that doesn't follow the spec strictly. And
when I tested in past, Windows also allowed to read the directory beyond
that limit. I can't recall though if there is in real case or just test
case though.
So if there is no strong reason to apply the limit, I don't think it is
good to limit it. (btw, the current code should detect the corruption of
infinite loop already)
Thanks.
> ---
> fs/fat/inode.c | 7 +++
> 1 file changed, 7 insertions(+)
>
> diff --git a/fs/fat/inode.c b/fs/fat/inode.c
> index a0cf99d..9b2e81e 100644
> --- a/fs/fat/inode.c
> +++ b/fs/fat/inode.c
> @@ -490,6 +490,13 @@ static int fat_calc_dir_size(struct inode *inode)
> return ret;
> inode->i_size = (fclus + 1) << sbi->cluster_bits;
>
> + if (i_size_read(inode) > FAT_MAX_DIR_SIZE) {
> + fat_fs_error(inode->i_sb,
> + "%s corrupted directory (invalid size %lld)\n",
> + __func__, i_size_read(inode));
> + return -EIO;
> + }
> +
> return 0;
> }
--
OGAWA Hirofumi
Re: [PATCH] fatfs: switch write_lock to read_lock in fat_ioctl_get_attributes
fengyubo writes:
> From: Yubo Feng
>
> There is no necessery to hold write_lock in fat_ioctl_get_attributes.
> write_lock may make an impact on concurrency of fat_ioctl_get_attributes.
>
> Signed-off-by: Yubo Feng
Looks good.
Acked-by: OGAWA Hirofumi
> ---
> fs/fat/file.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fs/fat/file.c b/fs/fat/file.c
> index 42134c5..f9ee27c 100644
> --- a/fs/fat/file.c
> +++ b/fs/fat/file.c
> @@ -25,9 +25,9 @@ static int fat_ioctl_get_attributes(struct inode *inode,
> u32 __user *user_attr)
> {
> u32 attr;
>
> - inode_lock(inode);
> + inode_lock_shared(inode);
> attr = fat_make_attrs(inode);
> - inode_unlock(inode);
> + inode_unlock_shared(inode);
>
> return put_user(attr, user_attr);
> }
--
OGAWA Hirofumi
Re: [PATCH] fat: add a check to fat_add_new_entries
t...@redhat.com writes:
> start_blknr = blknr = fat_clus_to_blknr(sbi, cluster[i]);
> last_blknr = start_blknr + sbi->sec_per_clus;
> +
> + /* overflow */
> + if (unlikely(last_blknr <= start_blknr)) {
> + err = -ENOMEM;
> + goto error_nomem;
> + }
> +
The cluster is 28bits and sec_per_clus is 8bits, so this should never
overflow actually. Is there no way to tell it to clang?
Thanks.
--
OGAWA Hirofumi
Re: [PATCH] drivers/staging/exfat - by default, prohibit mount of fat/vfat
Greg Kroah-Hartman writes: > On Mon, Sep 02, 2019 at 03:00:17PM -0400, Valdis Klētnieks wrote: >> On Mon, 02 Sep 2019 17:25:24 +0200, Greg Kroah-Hartman said: >> >> > I dug up my old discussion with the current vfat maintainer and he said >> > something to the affect of, "leave the existing code alone, make a new >> > filesystem, I don't want anything to do with exfat". >> > >> > And I don't blame them, vfat is fine as-is and stable and shouldn't be >> > touched for new things. >> > >> > We can keep non-vfat filesystems from being mounted with the exfat >> > codebase, and make things simpler for everyone involved. >> >> Ogawa: >> >> Is this still your position, that you want exfat to be a separate module? > > Personally I agree that this should be separate at least for quite some > time to shake things out at the very least. But I'll defer to Ogawa if > he thinks things should be merged. I'm not reading whole of this thread, so I can be pointless though. I can't recall the discussion of exfat with you. My history about exfat is, write read-only exfat from on-disk data -> MS published patent to their site or such -> stopped about exfat -> recently looks like MS changed mind Well, if you are going to developing actively, IMO it would be better to drop historically bad decisions in fat driver (some stuff would be hard to fix without user visible changes), and re-think from basic implementation design. And I can't recall the detail of exfat format though, IIRC, the common code is not so big, but some stuff can be shared with fat (timestamp stuff, fatent stuff, IIRC). So IMO it is better to be different driver basically, however on other hand, it is better to share the code for same on-disk format if possible. Anyway, I don't have strong opinion about it. Thanks. -- OGAWA Hirofumi
Re: [PATCH] fat: Delete an unnecessary check before brelse()
Markus Elfring writes: > From: Markus Elfring > Date: Tue, 3 Sep 2019 14:56:16 +0200 > > The brelse() function tests whether its argument is NULL > and then returns immediately. > Thus the test around the call is not needed. > > This issue was detected by using the Coccinelle software. > > Signed-off-by: Markus Elfring Acked-by: OGAWA Hirofumi Thanks. > --- > fs/fat/dir.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/fs/fat/dir.c b/fs/fat/dir.c > index 1bda2ab6745b..f4bc87a3c98d 100644 > --- a/fs/fat/dir.c > +++ b/fs/fat/dir.c > @@ -88,9 +88,7 @@ static int fat__get_entry(struct inode *dir, loff_t *pos, > int err, offset; > > next: > - if (*bh) > - brelse(*bh); > - > + brelse(*bh); > *bh = NULL; > iblock = *pos >> sb->s_blocksize_bits; > err = fat_bmap(dir, iblock, , _blocks, 0, false); > -- > 2.23.0 > -- OGAWA Hirofumi
Re: [PATCH] fat: fix corruption in fat_alloc_new_dir()
Jan Stancek writes:
> sb_getblk does not guarantee that buffer_head is uptodate. If there is
> async read running in parallel for same buffer_head, it can overwrite
> just initialized msdos_dir_entry, leading to corruption:
> FAT-fs (loop0): error, corrupted directory (invalid entries)
> FAT-fs (loop0): Filesystem has been set read-only
>
> This can happen for example during LTP statx04, which creates loop
> device, formats it (mkfs.vfat), mounts it and immediately creates
> a new directory. In parallel, systemd-udevd is probing new block
> device, which leads to async read.
>
> do_mkdirat ksys_read
>vfs_mkdir vfs_read
> vfat_mkdir __vfs_read
> fat_alloc_new_dir new_sync_read
>/* init de[0], de[1] */blkdev_read_iter
>generic_file_read_iter
> generic_file_buffered_read
> blkdev_readpage
> block_read_full_page
>
> Faster reproducer (based on LTP statx04):
>
> int main(void)
> {
> int i, j, ret, fd, loop_fd, ctrl_fd;
> int loop_num;
> char loopdev[256], tmp[256], testfile[256];
>
> mkdir("/tmp/mntpoint", 0777);
> for (i = 0; ; i++) {
> printf("Iteration: %d\n", i);
> sprintf(testfile, "/tmp/test.img.%d", getpid());
>
> ctrl_fd = open("/dev/loop-control", O_RDWR);
> loop_num = ioctl(ctrl_fd, LOOP_CTL_GET_FREE);
> close(ctrl_fd);
> sprintf(loopdev, "/dev/loop%d", loop_num);
>
> fd = open(testfile, O_WRONLY|O_CREAT|O_TRUNC, 0600);
> fallocate(fd, 0, 0, 256*1024*1024);
> close(fd);
>
> fd = open(testfile, O_RDWR);
> loop_fd = open(loopdev, O_RDWR);
> ioctl(loop_fd, LOOP_SET_FD, fd);
> close(loop_fd);
> close(fd);
>
> sprintf(tmp, "mkfs.vfat %s", loopdev);
> system(tmp);
> mount(loopdev, "/tmp/mntpoint", "vfat", 0, NULL);
>
> for (j = 0; j < 200; j++) {
> sprintf(tmp, "/tmp/mntpoint/testdir%d", j);
> ret = mkdir(tmp, 0777);
> if (ret) {
> perror("mkdir");
> break;
> }
> }
>
> umount("/tmp/mntpoint");
> loop_fd = open(loopdev, O_RDWR);
> ioctl(loop_fd, LOOP_CLR_FD, fd);
> close(loop_fd);
> unlink(testfile);
>
> if (ret)
> break;
> }
>
> return 0;
> }
>
> Issue triggers within minute on HPE Apollo 70 (arm64, 64GB RAM, 224 CPUs).
Using the device while mounting same device doesn't work reliably like
this race. (getblk() is intentionally used to get the buffer to write
new data.)
mount(2) internally opens the device by EXCL mode, so I guess udev opens
without EXCL (I dont know if it is intent or not).
Thanks.
--
OGAWA Hirofumi
Re: [ANNOUNCE] Three things.
Daniel Phillips writes: > Code for Tux3 is here: > > https://github.com/OGAWAHirofumi/tux3/tree/hirofumi It's old repo, actually the git repo of current code is, kernel https://github.com/OGAWAHirofumi/linux-tux3/tree/hirofumi userspace https://github.com/OGAWAHirofumi/linux-tux3/tree/hirofumi-user Thanks. -- OGAWA Hirofumi
Re: [PATCH 12/20] fs: fat: Initialize filesystem timestamp ranges
Deepa Dinamani writes:
>> At least, it is wrong to call fat_time_fat2unix() before setup parameters
>> in sbi.
>
> All the parameters that fat_time_fat2unix() cares in sbi is accessed through
>
> static inline int fat_tz_offset(struct msdos_sb_info *sbi)
> {
> return (sbi->options.tz_set ?
>-sbi->options.time_offset :
>sys_tz.tz_minuteswest) * SECS_PER_MIN;
> }
>
> Both the sbi fields sbi->options.tz_set and sbi->options.time_offset
> are set by the call to parse_options(). And, parse_options() is called
> before the calls to fat_time_fat2unix().:
>
> int fat_fill_super(struct super_block *sb, void *data, int silent, int isvfat,
>void (*setup)(struct super_block *))
> {
>
>
> error = parse_options(sb, data, isvfat, silent, , >options);
> if (error)
> goto out_fail;
>
>
>
> sbi->prev_free = FAT_START_ENT;
> sb->s_maxbytes = 0x;
> fat_time_fat2unix(sbi, , 0, cpu_to_le16(FAT_DATE_MIN), 0);
> sb->s_time_min = ts.tv_sec;
>
> fat_time_fat2unix(sbi, , cpu_to_le16(FAT_TIME_MAX),
> cpu_to_le16(FAT_DATE_MAX), 0);
> sb->s_time_max = ts.tv_sec;
>
>
> }
>
> I do not see what the problem is.
Ouch, you are right. I was reading that patch wrongly, sorry.
Thanks.
--
OGAWA Hirofumi
Re: [PATCH 12/20] fs: fat: Initialize filesystem timestamp ranges
Deepa Dinamani writes:
> +/* DOS dates from 1980/1/1 through 2107/12/31 */
> +#define FAT_DATE_MIN (0<<9 | 1<<5 | 1)
> +#define FAT_DATE_MAX (127<<9 | 12<<5 | 31)
> +#define FAT_TIME_MAX (23<<11 | 59<<5 | 29)
> +
> /*
> * A deserialized copy of the on-disk structure laid out in struct
> * fat_boot_sector.
> @@ -1605,6 +1610,7 @@ int fat_fill_super(struct super_block *sb, void *data,
> int silent, int isvfat,
> int debug;
> long error;
> char buf[50];
> + struct timespec64 ts;
>
> /*
>* GFP_KERNEL is ok here, because while we do hold the
> @@ -1698,6 +1704,12 @@ int fat_fill_super(struct super_block *sb, void *data,
> int silent, int isvfat,
> sbi->free_clus_valid = 0;
> sbi->prev_free = FAT_START_ENT;
> sb->s_maxbytes = 0x;
> + fat_time_fat2unix(sbi, , 0, cpu_to_le16(FAT_DATE_MIN), 0);
> + sb->s_time_min = ts.tv_sec;
> +
> + fat_time_fat2unix(sbi, , cpu_to_le16(FAT_TIME_MAX),
> + cpu_to_le16(FAT_DATE_MAX), 0);
> + sb->s_time_max = ts.tv_sec;
At least, it is wrong to call fat_time_fat2unix() before setup parameters
in sbi.
And please move those timestamp stuff to fat/misc.c like other fat
timestamp helpers. (Maybe, provide fat_time_{min,max}() from fat/misc.c,
or fat_init_time() such?).
Thanks.
--
OGAWA Hirofumi
Re: [PATCH] fat: Add nobarrier to workaround the strange behavior of device
Christoph Hellwig writes: > On Sat, Jun 29, 2019 at 12:03:46AM +0900, OGAWA Hirofumi wrote: >> I see, sounds like good though. Does it work for all stable versions? >> Can it disable only flush command without other effect? And it would be >> better to be normal user controllable easily. > > The option was added in 2.6.17, so it's been around forever. But > no, it obviously is not user exposed as using it on a normal drive > can lead to data loss. I see. It sounds like good as long term solution though (if no effect other than disabling flush command), it may need some monitor daemon and detect the device, and apply user policy as root. (BTW, I meant, workaround is normal user controllable with config by root or such) I think, it may not be good as short term solution for especially stable users. If there is no better short term solution, I would like to still push this patch as short term workaround. Thanks. -- OGAWA Hirofumi
Re: [PATCH] fat: Add nobarrier to workaround the strange behavior of device
Christoph Hellwig writes: > On Fri, Jun 28, 2019 at 11:18:19PM +0900, OGAWA Hirofumi wrote: >> To workaround those devices and provide flexibility, this adds >> "barrier"/"nobarrier" mount options to fat driver. > > We have deprecated these rather misnamed options, and now instead allow > tweaking the 'cache_type' attribute on the SCSI device. I see, sounds like good though. Does it work for all stable versions? Can it disable only flush command without other effect? And it would be better to be normal user controllable easily. This happened on normal user's calibre app that mount via udisks. With this option, user can workaround with /etc/fstab for immediate users. > That being said if the device behave this buggy you should also report > it to to the usb-storage and scsi maintainers so that we can add a > quirk for it. It might not be able to say as buggy simply. The device looks work if no idle and not hit pattern of usage, so quirk can be overkill. Anyway, I don't have the device, if you can get the device and investigate, it can be fine. Thanks. -- OGAWA Hirofumi
[PATCH v2] fat: Add nobarrier to workaround the strange behavior of device
v2:
Just cleanup, changed the place of options under comment of fat.
---
There was the report of strange behavior of device with recent
blkdev_issue_flush() position change.
The following is simplified usbmon trace.
4203 9.160230 host -> 1.25.1 USBMS 95 SCSI: Synchronize
Cache(10) LUN: 0x00 (LBA: 0x, Len: 0)
4206 9.164911 1.25.1 -> host USBMS 77 SCSI: Response LUN: 0x00
(Synchronize Cache(10)) (Good)
4207 9.323927 host -> 1.25.1 USBMS 95 SCSI: Read(10) LUN: 0x00
(LBA: 0x00279950, Len: 240)
4212 9.327138 1.25.1 -> host USBMS 77 SCSI: Response LUN: 0x00
(Read(10)) (Good)
[...]
7323 10.202167 host -> 1.25.1 USBMS 95 SCSI: Synchronize
Cache(10) LUN: 0x00 (LBA: 0x, Len: 0)
7326 10.432266 1.25.1 -> host USBMS 77 SCSI: Response LUN: 0x00
(Synchronize Cache(10)) (Good)
7327 10.769092 host -> 1.25.1 USBMS 95 SCSI: Test Unit Ready
LUN: 0x00
7330 10.769192 1.25.1 -> host USBMS 77 SCSI: Response LUN: 0x00
(Test Unit Ready) (Good)
7335 12.849093 host -> 1.25.1 USBMS 95 SCSI: Test Unit Ready
LUN: 0x00
7338 12.849206 1.25.1 -> host USBMS 77 SCSI: Response LUN: 0x00
(Test Unit Ready) (Check Condition)
7339 12.849209 host -> 1.25.1 USBMS 95 SCSI: Request Sense LUN:
0x00
If "Synchronize Cache" command issued then there is idle time, the
device stop to process further commands, and behave as like no media.
(it returns NOT_READY [MEDIUM NOT PRESENT] for SENSE command, and this
happened on Kindle) [just a guess, the device is trying to detect the
"safe-unplug" operation of Windows or such?]
To workaround those devices and provide flexibility, this adds
"barrier"/"nobarrier" mount options to fat driver.
Cc:
Signed-off-by: OGAWA Hirofumi
---
fs/fat/fat.h |1 +
fs/fat/file.c |8 ++--
fs/fat/inode.c | 22 +-
3 files changed, 24 insertions(+), 7 deletions(-)
diff -puN fs/fat/fat.h~fat-nobarrier fs/fat/fat.h
--- linux/fs/fat/fat.h~fat-nobarrier2019-06-28 21:22:18.146191739 +0900
+++ linux-hirofumi/fs/fat/fat.h 2019-06-28 23:26:04.881215721 +0900
@@ -51,6 +51,7 @@ struct fat_mount_options {
tz_set:1, /* Filesystem timestamps' offset set */
rodir:1, /* allow ATTR_RO for directory */
discard:1,/* Issue discard requests on deletions */
+barrier:1,/* Issue FLUSH command */
dos1xfloppy:1;/* Assume default BPB for DOS 1.x floppies */
};
diff -puN fs/fat/file.c~fat-nobarrier fs/fat/file.c
--- linux/fs/fat/file.c~fat-nobarrier 2019-06-28 21:22:18.147191734 +0900
+++ linux-hirofumi/fs/fat/file.c2019-06-28 23:26:04.881215721 +0900
@@ -193,17 +193,21 @@ static int fat_file_release(struct inode
int fat_file_fsync(struct file *filp, loff_t start, loff_t end, int datasync)
{
struct inode *inode = filp->f_mapping->host;
+ struct msdos_sb_info *sbi = MSDOS_SB(inode->i_sb);
int err;
err = __generic_file_fsync(filp, start, end, datasync);
if (err)
return err;
- err = sync_mapping_buffers(MSDOS_SB(inode->i_sb)->fat_inode->i_mapping);
+ err = sync_mapping_buffers(sbi->fat_inode->i_mapping);
if (err)
return err;
- return blkdev_issue_flush(inode->i_sb->s_bdev, GFP_KERNEL, NULL);
+ if (sbi->options.barrier)
+ err = blkdev_issue_flush(inode->i_sb->s_bdev, GFP_KERNEL, NULL);
+
+ return err;
}
diff -puN fs/fat/inode.c~fat-nobarrier fs/fat/inode.c
--- linux/fs/fat/inode.c~fat-nobarrier 2019-06-28 21:22:18.148191730 +0900
+++ linux-hirofumi/fs/fat/inode.c 2019-06-28 23:26:28.029103863 +0900
@@ -1016,6 +1016,8 @@ static int fat_show_options(struct seq_f
seq_puts(m, ",nfs=stale_rw");
if (opts->discard)
seq_puts(m, ",discard");
+ if (!opts->barrier)
+ seq_puts(m, ",nobarrier");
if (opts->dos1xfloppy)
seq_puts(m, ",dos1xfloppy");
@@ -1031,8 +1033,9 @@ enum {
Opt_shortname_winnt, Opt_shortname_mixed, Opt_utf8_no, Opt_utf8_yes,
Opt_uni_xl_no, Opt_uni_xl_yes, Opt_nonumtail_no, Opt_nonumtail_yes,
Opt_obsolete, Opt_flush, Opt_tz_utc, Opt_rodir, Opt_err_cont,
- Opt_err_panic, Opt_err_ro, Opt_discard, Opt_nfs, Opt_time_offset,
- Opt_nfs_stale_rw, Opt_nfs_nostale_ro, Opt_err, Opt_dos1xfloppy,
+ Opt_err_panic, Opt_err_ro, Opt_discard, Opt_barrier, Opt_nobarrier,
+ Opt_nfs, Opt_time_offset, Opt_nfs_stale_rw, Opt_nfs_nostale_ro,
+ Opt_err, Opt_dos1xfloppy,
};
static const match_table_t fat_tokens = {
@@ -1062,6 +1065,8 @@ static const matc
[PATCH] fat: Add nobarrier to workaround the strange behavior of device
There was the report of strange behavior of device with recent
blkdev_issue_flush() position change.
The following is simplified usbmon trace.
4203 9.160230 host -> 1.25.1 USBMS 95 SCSI: Synchronize
Cache(10) LUN: 0x00 (LBA: 0x, Len: 0)
4206 9.164911 1.25.1 -> host USBMS 77 SCSI: Response LUN: 0x00
(Synchronize Cache(10)) (Good)
4207 9.323927 host -> 1.25.1 USBMS 95 SCSI: Read(10) LUN: 0x00
(LBA: 0x00279950, Len: 240)
4212 9.327138 1.25.1 -> host USBMS 77 SCSI: Response LUN: 0x00
(Read(10)) (Good)
[...]
7323 10.202167 host -> 1.25.1 USBMS 95 SCSI: Synchronize
Cache(10) LUN: 0x00 (LBA: 0x, Len: 0)
7326 10.432266 1.25.1 -> host USBMS 77 SCSI: Response LUN: 0x00
(Synchronize Cache(10)) (Good)
7327 10.769092 host -> 1.25.1 USBMS 95 SCSI: Test Unit Ready
LUN: 0x00
7330 10.769192 1.25.1 -> host USBMS 77 SCSI: Response LUN: 0x00
(Test Unit Ready) (Good)
7335 12.849093 host -> 1.25.1 USBMS 95 SCSI: Test Unit Ready
LUN: 0x00
7338 12.849206 1.25.1 -> host USBMS 77 SCSI: Response LUN: 0x00
(Test Unit Ready) (Check Condition)
7339 12.849209 host -> 1.25.1 USBMS 95 SCSI: Request Sense LUN:
0x00
If "Synchronize Cache" command issued then there is idle time, the
device stop to process further commands, and behave as like no media.
(it returns NOT_READY [MEDIUM NOT PRESENT] for SENSE command, and this
happened on Kindle) [just a guess, the device is trying to detect the
"safe-unplug" operation of Windows or such?]
To workaround those devices and provide flexibility, this adds
"barrier"/"nobarrier" mount options to fat driver.
Cc:
Signed-off-by: OGAWA Hirofumi
---
fs/fat/fat.h |1 +
fs/fat/file.c |8 ++--
fs/fat/inode.c | 16 ++--
3 files changed, 21 insertions(+), 4 deletions(-)
diff -puN fs/fat/fat.h~fat-nobarrier fs/fat/fat.h
--- linux/fs/fat/fat.h~fat-nobarrier2019-06-28 21:22:18.146191739 +0900
+++ linux-hirofumi/fs/fat/fat.h 2019-06-28 21:59:11.693934616 +0900
@@ -51,6 +51,7 @@ struct fat_mount_options {
tz_set:1, /* Filesystem timestamps' offset set */
rodir:1, /* allow ATTR_RO for directory */
discard:1,/* Issue discard requests on deletions */
+barrier:1,/* Issue FLUSH command */
dos1xfloppy:1;/* Assume default BPB for DOS 1.x floppies */
};
diff -puN fs/fat/file.c~fat-nobarrier fs/fat/file.c
--- linux/fs/fat/file.c~fat-nobarrier 2019-06-28 21:22:18.147191734 +0900
+++ linux-hirofumi/fs/fat/file.c2019-06-28 21:59:11.693934616 +0900
@@ -193,17 +193,21 @@ static int fat_file_release(struct inode
int fat_file_fsync(struct file *filp, loff_t start, loff_t end, int datasync)
{
struct inode *inode = filp->f_mapping->host;
+ struct msdos_sb_info *sbi = MSDOS_SB(inode->i_sb);
int err;
err = __generic_file_fsync(filp, start, end, datasync);
if (err)
return err;
- err = sync_mapping_buffers(MSDOS_SB(inode->i_sb)->fat_inode->i_mapping);
+ err = sync_mapping_buffers(sbi->fat_inode->i_mapping);
if (err)
return err;
- return blkdev_issue_flush(inode->i_sb->s_bdev, GFP_KERNEL, NULL);
+ if (sbi->options.barrier)
+ err = blkdev_issue_flush(inode->i_sb->s_bdev, GFP_KERNEL, NULL);
+
+ return err;
}
diff -puN fs/fat/inode.c~fat-nobarrier fs/fat/inode.c
--- linux/fs/fat/inode.c~fat-nobarrier 2019-06-28 21:22:18.148191730 +0900
+++ linux-hirofumi/fs/fat/inode.c 2019-06-28 21:59:11.694934611 +0900
@@ -1016,6 +1016,8 @@ static int fat_show_options(struct seq_f
seq_puts(m, ",nfs=stale_rw");
if (opts->discard)
seq_puts(m, ",discard");
+ if (!opts->barrier)
+ seq_puts(m, ",nobarrier");
if (opts->dos1xfloppy)
seq_puts(m, ",dos1xfloppy");
@@ -1031,8 +1033,9 @@ enum {
Opt_shortname_winnt, Opt_shortname_mixed, Opt_utf8_no, Opt_utf8_yes,
Opt_uni_xl_no, Opt_uni_xl_yes, Opt_nonumtail_no, Opt_nonumtail_yes,
Opt_obsolete, Opt_flush, Opt_tz_utc, Opt_rodir, Opt_err_cont,
- Opt_err_panic, Opt_err_ro, Opt_discard, Opt_nfs, Opt_time_offset,
- Opt_nfs_stale_rw, Opt_nfs_nostale_ro, Opt_err, Opt_dos1xfloppy,
+ Opt_err_panic, Opt_err_ro, Opt_discard, Opt_barrier, Opt_nobarrier,
+ Opt_nfs, Opt_time_offset, Opt_nfs_stale_rw, Opt_nfs_nostale_ro,
+ Opt_err, Opt_dos1xfloppy,
};
static const match_table_t fat_tokens = {
@@ -1062,6 +1065,8 @@ static const match_table_t fat_tokens =
{Opt_err_panic, "errors=panic"},
{
Re: [PATCH] fat: issue flush after the writeback of FAT
Hou Tao writes:
> fsync() needs to make sure the data & meta-data of file are persistent
> after the return of fsync(), even when a power-failure occurs later.
> In the case of fat-fs, the FAT belongs to the meta-data of file,
> so we need to issue a flush after the writeback of FAT instead before.
>
> Also bail out early when any stage of fsync fails.
>
> Signed-off-by: Hou Tao
Looks good.
Acked-by: OGAWA Hirofumi
> ---
> fs/fat/file.c | 11 ---
> 1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/fs/fat/file.c b/fs/fat/file.c
> index b3bed32946b1..0e3ed79fcc3f 100644
> --- a/fs/fat/file.c
> +++ b/fs/fat/file.c
> @@ -193,12 +193,17 @@ static int fat_file_release(struct inode *inode, struct
> file *filp)
> int fat_file_fsync(struct file *filp, loff_t start, loff_t end, int datasync)
> {
> struct inode *inode = filp->f_mapping->host;
> - int res, err;
> + int err;
> +
> + err = __generic_file_fsync(filp, start, end, datasync);
> + if (err)
> + return err;
>
> - res = generic_file_fsync(filp, start, end, datasync);
> err = sync_mapping_buffers(MSDOS_SB(inode->i_sb)->fat_inode->i_mapping);
> + if (err)
> + return err;
>
> - return res ? res : err;
> + return blkdev_issue_flush(inode->i_sb->s_bdev, GFP_KERNEL, NULL);
> }
--
OGAWA Hirofumi
Re: [PATCH] fat: issue flush after the writeback of FAT
"Darrick J. Wong" writes: >> +err = __generic_file_fsync(filp, start, end, datasync); >> +if (err) >> +return err; >> >> -res = generic_file_fsync(filp, start, end, datasync); >> err = sync_mapping_buffers(MSDOS_SB(inode->i_sb)->fat_inode->i_mapping); > > Huh. I would've thought that flushing the FAT would also be required > at the end of a WB_SYNC_ALL (aka data integrity) writepages call? In fatfs implement, FAT area is flushed by sync_mapping_buffers(fat_inode). (FAT buffer is dirtied only by using bh->b_assoc_map to fat_inode) Thanks. -- OGAWA Hirofumi
Re: [PATCH v3] vfat: don't read garbage after last dirent
Matteo Croce writes:
> The FAT32 File System Specification[1] states that:
>
> If DIR_Name[0] == 0x00, then the directory entry is free, and there
> are no allocated directory entries after this one.
>
> The special 0 value, indicates to FAT file system driver code that
> the rest of the entries in this directory do not need to be examined
> because they are all free.
>
> This is not enforced by Linux, and is possible to read garbage if not
> all dirents after the last one are filled with zeroes.
>
> [1]
> http://download.microsoft.com/download/1/6/1/161ba512-40e2-4cc9-843a-923143f3456c/fatgen103.doc
Tested some, and this patch seems to be not working. First
fat_readdir() exits by zero dir entry, but dir pos is updated to next
position, then second fat_readdir() continue with previous dir pos and
will read beyond zero dir entry.
So, maybe, we are better move down the zero dir entry handling into
fat_get_entry(), instead of copying several places. But like you know,
we would like to skip the zero dir entry only if read/search operation.
Maybe, adding flag to fat_get_entry() and use it, or adding small
wrapper? And in read/search case, we should not update *pos if hit the
zero dir entry.
I'm not testing the above actually though, we would need such.
Thanks.
> Reported-by: Timothy Redaelli
> Signed-off-by: Matteo Croce
> ---
> fs/fat/dir.c | 20
> 1 file changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/fs/fat/dir.c b/fs/fat/dir.c
> index 9d01db37183f..d919a1ee519c 100644
> --- a/fs/fat/dir.c
> +++ b/fs/fat/dir.c
> @@ -314,7 +314,7 @@ static int fat_parse_long(struct inode *dir, loff_t *pos,
>
> if (ds->id & 0x40)
> (*unicode)[offset + 13] = 0;
> - if (fat_get_entry(dir, pos, bh, de) < 0)
> + if (fat_get_entry(dir, pos, bh, de) < 0 || !(*de)->name[0])
> return PARSE_EOF;
> if (slot == 0)
> break;
> @@ -476,7 +476,8 @@ int fat_search_long(struct inode *inode, const unsigned
> char *name,
>
> err = -ENOENT;
> while (1) {
> - if (fat_get_entry(inode, , , ) == -1)
> + if (fat_get_entry(inode, , , ) == -1 ||
> + !de->name[0])
> goto end_of_dir;
> parse_record:
> nr_slots = 0;
> @@ -588,7 +589,7 @@ static int __fat_readdir(struct inode *inode, struct file
> *file,
>
> bh = NULL;
> get_new:
> - if (fat_get_entry(inode, , , ) == -1)
> + if (fat_get_entry(inode, , , ) == -1 || !de->name[0])
> goto end_of_dir;
> parse_record:
> nr_slots = 0;
> @@ -898,7 +899,8 @@ int fat_get_dotdot_entry(struct inode *dir, struct
> buffer_head **bh,
> loff_t offset = 0;
>
> *de = NULL;
> - while (fat_get_short_entry(dir, , bh, de) >= 0) {
> + while (fat_get_short_entry(dir, , bh, de) >= 0 &&
> +(*de)->name[0]) {
> if (!strncmp((*de)->name, MSDOS_DOTDOT, MSDOS_NAME))
> return 0;
> }
> @@ -916,7 +918,8 @@ int fat_dir_empty(struct inode *dir)
>
> bh = NULL;
> cpos = 0;
> - while (fat_get_short_entry(dir, , , ) >= 0) {
> + while (fat_get_short_entry(dir, , , ) >= 0 &&
> +de->name[0]) {
> if (strncmp(de->name, MSDOS_DOT , MSDOS_NAME) &&
> strncmp(de->name, MSDOS_DOTDOT, MSDOS_NAME)) {
> result = -ENOTEMPTY;
> @@ -941,7 +944,8 @@ int fat_subdirs(struct inode *dir)
>
> bh = NULL;
> cpos = 0;
> - while (fat_get_short_entry(dir, , , ) >= 0) {
> + while (fat_get_short_entry(dir, , , ) >= 0 &&
> +de->name[0]) {
> if (de->attr & ATTR_DIR)
> count++;
> }
> @@ -961,7 +965,7 @@ int fat_scan(struct inode *dir, const unsigned char *name,
> sinfo->slot_off = 0;
> sinfo->bh = NULL;
> while (fat_get_short_entry(dir, >slot_off, >bh,
> ->de) >= 0) {
> +>de) >= 0 && sinfo->de->name[0]) {
> if (!strncmp(sinfo->de->name, name, MSDOS_NAME)) {
> sinfo->slot_off -= sizeof(*sinfo->de);
> sinfo->nr_slots = 1;
> @@ -985,7 +989,7 @@ int fat_scan_logstart(struct inode *dir, int i_logstart,
> sinfo->slot_off = 0;
> sinfo->bh = NULL;
> while (fat_get_short_entry(dir, >slot_off, >bh,
> ->de) >= 0) {
> +>de) >= 0 && sinfo->de->name[0]) {
> if (fat_get_start(MSDOS_SB(sb), sinfo->de) == i_logstart) {
> sinfo->slot_off -= sizeof(*sinfo->de);
> sinfo->nr_slots = 1;
--
OGAWA Hirofumi
Re: [PATCH] fat: enable .splice_write to support splice on O_DIRECT file
OGAWA Hirofumi writes:
> Hou Tao writes:
>
>> Now splice() on O_DIRECT-opened fat file will return -EFAULT, that is
>> because the default .splice_write, namely default_file_splice_write(),
>> will construct an ITER_KVEC iov_iter and dio_refill_pages() in dio path
>> can not handle it.
>>
>> Fix it by implementing .splice_write through iter_file_splice_write().
>>
>> Spotted by xfs-tests generic/091.
>>
>> Signed-off-by: Hou Tao
>> ---
>> fs/fat/file.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/fs/fat/file.c b/fs/fat/file.c
>> index 13935ee99e1e..b3bed32946b1 100644
>> --- a/fs/fat/file.c
>> +++ b/fs/fat/file.c
>> @@ -214,6 +214,7 @@ const struct file_operations fat_file_operations = {
>> #endif
>> .fsync = fat_file_fsync,
>> .splice_read= generic_file_splice_read,
>> +.splice_write = iter_file_splice_write,
>> .fallocate = fat_fallocate,
>> };
>
> Looks good.
>
> Acked-by: OGAWA Hirofumi
>
> Thanks.
Forgot to include akpm to addresses.
--
OGAWA Hirofumi
Re: [PATCH] fat: enable .splice_write to support splice on O_DIRECT file
Hou Tao writes:
> Now splice() on O_DIRECT-opened fat file will return -EFAULT, that is
> because the default .splice_write, namely default_file_splice_write(),
> will construct an ITER_KVEC iov_iter and dio_refill_pages() in dio path
> can not handle it.
>
> Fix it by implementing .splice_write through iter_file_splice_write().
>
> Spotted by xfs-tests generic/091.
>
> Signed-off-by: Hou Tao
> ---
> fs/fat/file.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/fs/fat/file.c b/fs/fat/file.c
> index 13935ee99e1e..b3bed32946b1 100644
> --- a/fs/fat/file.c
> +++ b/fs/fat/file.c
> @@ -214,6 +214,7 @@ const struct file_operations fat_file_operations = {
> #endif
> .fsync = fat_file_fsync,
> .splice_read= generic_file_splice_read,
> + .splice_write = iter_file_splice_write,
> .fallocate = fat_fallocate,
> };
Looks good.
Acked-by: OGAWA Hirofumi
Thanks.
--
OGAWA Hirofumi
Re: [PATCH v2] vfat: don't read garbage after last dirent
Matteo Croce writes: > The FAT32 File System Specification[1] states that: > > If DIR_Name[0] == 0x00, then the directory entry is free, and there > are no allocated directory entries after this one. > > The special 0 value, indicates to FAT file system driver code that > the rest of the entries in this directory do not need to be examined > because they are all free. > > This is not enforced by Linux, and is possible to read garbage if not > all dirents after the last one are filled with zeroes. > > [1] > http://download.microsoft.com/download/1/6/1/161ba512-40e2-4cc9-843a-923143f3456c/fatgen103.doc > > Reported-by: Timothy Redaelli > Signed-off-by: Matteo Croce We have to handle all paths that is using fat_get_entry(), to make consistent behavior. With quick check, there are still several issues remaining. Please check more. For example, looks like fat_parse_long()/fat_search_long() path is missing, and fat_get_dotdot_entry(), fat_subdirs() too. (while adding new entry, if we found zeroed entry, we would be better to warn about fsck.) Thanks. -- OGAWA Hirofumi
Re: [PATCH v3 0/3] fat: Added functions to determine the FAT variant (12/16/32bit)
Carmeli Tamir writes: > Along the FAT FS code, the FAT variant (whether this is FAT12, FAT16 or > FAT32) is > determined by checking the fat_bits field of struct msdos_sb_info. > This is somewhat error prone as it forces the usage of magics (12, 16, 32) > multiple times in the code. > > This series replaces the places in which the variant is checked with three > inline functions - IS_FAT12, IS_FAT16 and IS_FAT16. > > The introduction of these simple inline functions makes a clearer API for > determining the variant, > rather than searching the code for some field in a struct, and therefore > increases the code's maintainability and readability. > > In addition, minor cleanups around code that checks for the FAT variant, > and fixed comments from v1 and v2. > > Carmeli Tamir (3): > Removed fat_first_ent > Moved and inlined MAX_FAT > IS_FAT functions > > fs/fat/cache.c| 2 +- > fs/fat/dir.c | 4 ++-- > fs/fat/fat.h | 30 +- > fs/fat/fatent.c | 16 +++- > fs/fat/inode.c| 26 +++--- > fs/fat/misc.c | 2 +- > include/uapi/linux/msdos_fs.h | 5 - > 7 files changed, 55 insertions(+), 30 deletions(-) FWIW, Acked-by: OGAWA Hirofumi for all of this patchset. Thanks. -- OGAWA Hirofumi
Re: [PATCH v2 3/3] fat: New inline functions to determine the FAT variant (32, 16 or 12)
fat_fs_error(sb, "invalid FAT variant, %u bits", sbi->fat_bits);
> }
> }
>
> @@ -310,7 +308,7 @@ static void mark_fsinfo_dirty(struct super_block *sb)
> {
> struct msdos_sb_info *sbi = MSDOS_SB(sb);
>
> - if (sb_rdonly(sb) || sbi->fat_bits != 32)
> + if (sb_rdonly(sb) || !IS_FAT32(sbi))
> return;
>
> __mark_inode_dirty(sbi->fsinfo_inode, I_DIRTY_SYNC);
> @@ -327,7 +325,7 @@ static inline int fat_ent_update_ptr(struct super_block
> *sb,
> /* Is this fatent's blocks including this entry? */
> if (!fatent->nr_bhs || bhs[0]->b_blocknr != blocknr)
> return 0;
> - if (sbi->fat_bits == 12) {
> + if (IS_FAT12(sbi)) {
> if ((offset + 1) < sb->s_blocksize) {
> /* This entry is on bhs[0]. */
> if (fatent->nr_bhs == 2) {
> diff --git a/fs/fat/inode.c b/fs/fat/inode.c
> index 708de6d..a84b61b 100644
> --- a/fs/fat/inode.c
> +++ b/fs/fat/inode.c
> @@ -686,7 +686,7 @@ static void fat_set_state(struct super_block *sb,
>
> b = (struct fat_boot_sector *) bh->b_data;
>
> - if (sbi->fat_bits == 32) {
> + if (IS_FAT32(sbi)) {
> if (set)
> b->fat32.state |= FAT_STATE_DIRTY;
> else
> @@ -1397,7 +1397,7 @@ static int fat_read_root(struct inode *inode)
> inode->i_mode = fat_make_mode(sbi, ATTR_DIR, S_IRWXUGO);
> inode->i_op = sbi->dir_ops;
> inode->i_fop = _dir_operations;
> - if (sbi->fat_bits == 32) {
> + if (IS_FAT32(sbi)) {
> MSDOS_I(inode)->i_start = sbi->root_cluster;
> error = fat_calc_dir_size(inode);
> if (error < 0)
> @@ -1424,7 +1424,7 @@ static unsigned long calc_fat_clusters(struct
> super_block *sb)
> struct msdos_sb_info *sbi = MSDOS_SB(sb);
>
> /* Divide first to avoid overflow */
> - if (sbi->fat_bits != 12) {
> + if (!IS_FAT12(sbi)) {
> unsigned long ent_per_sec = sb->s_blocksize * 8 / sbi->fat_bits;
> return ent_per_sec * sbi->fat_length;
> }
> @@ -1744,7 +1744,7 @@ int fat_fill_super(struct super_block *sb, void *data,
> int silent, int isvfat,
> }
>
> /* interpret volume ID as a little endian 32 bit integer */
> - if (sbi->fat_bits == 32)
> + if (IS_FAT32(sbi))
> sbi->vol_id = bpb.fat32_vol_id;
> else /* fat 16 or 12 */
> sbi->vol_id = bpb.fat16_vol_id;
> @@ -1770,11 +1770,11 @@ int fat_fill_super(struct super_block *sb, void
> *data, int silent, int isvfat,
>
> total_clusters = (total_sectors - sbi->data_start) / sbi->sec_per_clus;
>
> - if (sbi->fat_bits != 32)
> + if (!IS_FAT32(sbi))
> sbi->fat_bits = (total_clusters > MAX_FAT12) ? 16 : 12;
>
> /* some OSes set FAT_STATE_DIRTY and clean it on unmount. */
> - if (sbi->fat_bits == 32)
> + if (IS_FAT32(sbi))
> sbi->dirty = bpb.fat32_state & FAT_STATE_DIRTY;
> else /* fat 16 or 12 */
> sbi->dirty = bpb.fat16_state & FAT_STATE_DIRTY;
> diff --git a/fs/fat/misc.c b/fs/fat/misc.c
> index fce0a76..5368c6a 100644
> --- a/fs/fat/misc.c
> +++ b/fs/fat/misc.c
> @@ -64,7 +64,7 @@ int fat_clusters_flush(struct super_block *sb)
> struct buffer_head *bh;
> struct fat_boot_fsinfo *fsinfo;
>
> - if (sbi->fat_bits != 32)
> + if (!IS_FAT32(sbi))
> return 0;
>
> bh = sb_bread(sb, sbi->fsinfo_sector);
--
OGAWA Hirofumi
Re: [PATCH v2 2/3] fat: Moved MAX_FAT to fat.h and changed it to inline function
Carmeli Tamir writes:
> MAX_FAT is useless in msdos_fs.h, since it uses the MSDOS_SB function
> that is defined in fat.h. So really, this macro can be only called
> from code that already includes fat.h.
>
> Hence, this patch moves it to fat.h, right after MSDOS_SB is defined.
> I also changed it to an inline function in order to save the double call
> to MSDOS_SB. This was suggested by j...@perches.com in the previous
> version.
>
> This patch is required for the next in the series, in which the variant
> (whether this is FAT12, FAT16 or FAT32) checks are replaced with new
> macros.
Could you use lower case chars for inline functions? Yeah, MSDOS_SB() is
upper case though, it is historical reason.
Thanks.
> Signed-off-by: Carmeli Tamir
> ---
> fs/fat/fat.h | 9 +
> include/uapi/linux/msdos_fs.h | 2 --
> 2 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/fs/fat/fat.h b/fs/fat/fat.h
> index 4e1b2f6..11bc4a2 100644
> --- a/fs/fat/fat.h
> +++ b/fs/fat/fat.h
> @@ -142,6 +142,15 @@ static inline struct msdos_sb_info *MSDOS_SB(struct
> super_block *sb)
> return sb->s_fs_info;
> }
>
> +/* Maximum number of clusters */
> +static inline u32 MAX_FAT(struct super_block *sb)
> +{
> + struct msdos_sb_info *sbi = MSDOS_SB(sb);
> +
> + return sbi->fat_bits == 32 ? MAX_FAT32 :
> + sbi->fat_bits == 16 ? MAX_FAT16 : MAX_FAT12;
> +}
> +
> static inline struct msdos_inode_info *MSDOS_I(struct inode *inode)
> {
> return container_of(inode, struct msdos_inode_info, vfs_inode);
> diff --git a/include/uapi/linux/msdos_fs.h b/include/uapi/linux/msdos_fs.h
> index 833c707..a577389 100644
> --- a/include/uapi/linux/msdos_fs.h
> +++ b/include/uapi/linux/msdos_fs.h
> @@ -65,8 +65,6 @@
> #define MAX_FAT120xFF4
> #define MAX_FAT160xFFF4
> #define MAX_FAT320x0FF6
> -#define MAX_FAT(s) (MSDOS_SB(s)->fat_bits == 32 ? MAX_FAT32 : \
> - MSDOS_SB(s)->fat_bits == 16 ? MAX_FAT16 : MAX_FAT12)
>
> /* bad cluster mark */
> #define BAD_FAT120xFF7
--
OGAWA Hirofumi
Re: [PATCH 2/2] fat: New macros to determine the FAT variant (32, 16 or 12)
Tamir Carmeli writes: > I just want to make sure, is there a reason why I shouldn't delete > FAT_FIRST_ENT, as Joe Perches commented? FAT_FIRST_ENT() was used to check if fat spec compliance. But in real world, there are too many implementations that didn't follow spec. Well, so, now FAT_FIRST_ENT() is pointed only from comment. The reason is only for this comment. Thanks. -- OGAWA Hirofumi
Re: [PATCH] vfat: don't read garbage after last dirent
Matteo Croce writes: > The FAT32 File System Specification[1] states: > > If DIR_Name[0] == 0x00, then the directory entry is free, and there > are no allocated directory entries after this one. > > The special 0 value, indicates to FAT file system driver code that > the rest of the entries in this directory do notneed to be examined > because they are all free. > > This is not enforced by Linux, and is possible to read garbage if > not all the dirents after the last one are filled with zeroes. > > [1] > http://download.microsoft.com/download/1/6/1/161ba512-40e2-4cc9-843a-923143f3456c/fatgen103.doc > > Reported-by: Timothy Redaelli > Signed-off-by: Matteo Croce I know this spec. But name[0] == 0 means - the rest of the entries in this directory must be 0. Otherwise, overwriting zeroed entry by adding new dir entry, then shows garbage as dir entry. So "stop at zero" is just a optimization. On other hand, I know there is buggy formatters don't clear that garbage, and uses it on read-only storage area. So I will agree to supporting "stop at zero" though (and keep assuming dir is initialized with zero clear. i.e. don't add tricky workaround to support buggy formatters.), the implementation should handle it for whole path, not only readdir(). E.g. I recall lookup, dir empty check path, and maybe there are others I can't recall immediately. > get_new: > - if (fat_get_entry(inode, , , ) == -1) > + if (fat_get_entry(inode, , , ) == -1 || !de->name[0]) > goto end_of_dir; > parse_record: > nr_slots = 0; Thanks. -- OGAWA Hirofumi
Re: [PATCH 2/2] fat: New macros to determine the FAT variant (32, 16 or 12)
Joe Perches writes: >> >> -#define FAT_FIRST_ENT(s, x) ((MSDOS_SB(s)->fat_bits == 32 ? 0x0F00 >> : \ >> -MSDOS_SB(s)->fat_bits == 16 ? 0xFF00 : 0xF00) | (x)) >> +#define IS_FAT12(sbi) (sbi->fat_bits == 12) >> +#define IS_FAT16(sbi) (sbi->fat_bits == 16) >> +#define IS_FAT32(sbi) (sbi->fat_bits == 32) > > sbi should be parenthesized or perhaps better these should be > static inline bool functions Right, rather this is the bug (not hit yet though) that should be fixed. Thanks. -- OGAWA Hirofumi
Re: [PATCH] fat: Replaced 11 magic to MSDOS_NAME for volume label
Carmeli Tamir writes:
> The FAT file system volume label file stored in the root directory should
> match the volume label field in the FAT boot sector. As consequence, the
> max length of these fields ought to be the same. This patch replaces the
> magic '11' usef in the struct fat_boot_sector with MSDOS_NAME,
> which is used in struct msdos_dir_entry.
>
> Please check the following references:
> 1. Microsoft FAT specification 2005
> (http://read.pudn.com/downloads77/ebook/294884/FAT32%20Spec%20%28SDA%20Contribution%29.pdf).
> Search for 'volume label'.
> 2. Microsoft Extensible Firmware Initiative, FAT32 File System Specification
> (https://staff.washington.edu/dittrich/misc/fatgen103.pdf).
> Search for 'volume label'.
> 3. User space code that creates FAT filesystem
> sometimes uses MSDOS_NAME for the label, sometimes not.
> Search for 'if (memcmp(label, NO_NAME, MSDOS_NAME))'.
> I consider to make the same patch there as well.
> https://github.com/dosfstools/dosfstools/blob/master/src/mkfs.fat.c
>
> Signed-off-by: Carmeli Tamir
Reviewed-by: Johannes Thumshirn
Reviewed-by: Sergey Senozhatsky
Acked-by: OGAWA Hirofumi
Looks good. Thanks.
> ---
> include/uapi/linux/msdos_fs.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/include/uapi/linux/msdos_fs.h b/include/uapi/linux/msdos_fs.h
> index fde7537..1216e6c 100644
> --- a/include/uapi/linux/msdos_fs.h
> +++ b/include/uapi/linux/msdos_fs.h
> @@ -135,7 +135,7 @@ struct fat_boot_sector {
> for mount state. */
> __u8signature; /* extended boot signature */
> __u8vol_id[4]; /* volume ID */
> - __u8vol_label[11]; /* volume label */
> + __u8vol_label[MSDOS_NAME]; /* volume label */
> __u8fs_type[8]; /* file system type */
> /* other fields are not added here */
> } fat16;
> @@ -158,7 +158,7 @@ struct fat_boot_sector {
> for mount state. */
> __u8signature; /* extended boot signature */
> __u8vol_id[4]; /* volume ID */
> - __u8vol_label[11]; /* volume label */
> + __u8vol_label[MSDOS_NAME]; /* volume label */
> __u8fs_type[8]; /* file system type */
> /* other fields are not added here */
> } fat32;
--
OGAWA Hirofumi
Re: [PATCH] fat: Replaced 11 magic to MSDOS_NAME for volume label
Carmeli Tamir writes:
> The FAT file system volume label file stored in the root directory should
> match the volume label field in the FAT boot sector. As consequence, the
> max length of these fields ought to be the same. This patch replaces the
> magic '11' usef in the struct fat_boot_sector with MSDOS_NAME,
> which is used in struct msdos_dir_entry.
>
> Please check the following references:
> 1. Microsoft FAT specification 2005
> (http://read.pudn.com/downloads77/ebook/294884/FAT32%20Spec%20%28SDA%20Contribution%29.pdf).
> Search for 'volume label'.
> 2. Microsoft Extensible Firmware Initiative, FAT32 File System Specification
> (https://staff.washington.edu/dittrich/misc/fatgen103.pdf).
> Search for 'volume label'.
> 3. User space code that creates FAT filesystem
> sometimes uses MSDOS_NAME for the label, sometimes not.
> Search for 'if (memcmp(label, NO_NAME, MSDOS_NAME))'.
> I consider to make the same patch there as well.
> https://github.com/dosfstools/dosfstools/blob/master/src/mkfs.fat.c
>
> Signed-off-by: Carmeli Tamir
Reviewed-by: Johannes Thumshirn
Reviewed-by: Sergey Senozhatsky
Acked-by: OGAWA Hirofumi
Looks good. Thanks.
> ---
> include/uapi/linux/msdos_fs.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/include/uapi/linux/msdos_fs.h b/include/uapi/linux/msdos_fs.h
> index fde7537..1216e6c 100644
> --- a/include/uapi/linux/msdos_fs.h
> +++ b/include/uapi/linux/msdos_fs.h
> @@ -135,7 +135,7 @@ struct fat_boot_sector {
> for mount state. */
> __u8signature; /* extended boot signature */
> __u8vol_id[4]; /* volume ID */
> - __u8vol_label[11]; /* volume label */
> + __u8vol_label[MSDOS_NAME]; /* volume label */
> __u8fs_type[8]; /* file system type */
> /* other fields are not added here */
> } fat16;
> @@ -158,7 +158,7 @@ struct fat_boot_sector {
> for mount state. */
> __u8signature; /* extended boot signature */
> __u8vol_id[4]; /* volume ID */
> - __u8vol_label[11]; /* volume label */
> + __u8vol_label[MSDOS_NAME]; /* volume label */
> __u8fs_type[8]; /* file system type */
> /* other fields are not added here */
> } fat32;
--
OGAWA Hirofumi
Re: [PATCH] fs/fat: add cond_resched to fat_count_free_clusters
Khazhismel Kumykov writes: > On non-preempt kernels this loop can take a long time (more than 50 > ticks) processing through entries. > > Signed-off-by: Khazhismel Kumykov > --- > fs/fat/fatent.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/fs/fat/fatent.c b/fs/fat/fatent.c > index defc2168de91..f58c0cacc531 100644 > --- a/fs/fat/fatent.c > +++ b/fs/fat/fatent.c > @@ -682,6 +682,7 @@ int fat_count_free_clusters(struct super_block *sb) > if (ops->ent_get() == FAT_ENT_FREE) > free++; > } while (fat_ent_next(sbi, )); > + cond_resched(); > } > sbi->free_clusters = free; > sbi->free_clus_valid = 1; Acked-by: OGAWA Hirofumi Thanks, looks good. -- OGAWA Hirofumi
Re: [PATCH] fs/fat: add cond_resched to fat_count_free_clusters
Khazhismel Kumykov writes: > On non-preempt kernels this loop can take a long time (more than 50 > ticks) processing through entries. > > Signed-off-by: Khazhismel Kumykov > --- > fs/fat/fatent.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/fs/fat/fatent.c b/fs/fat/fatent.c > index defc2168de91..f58c0cacc531 100644 > --- a/fs/fat/fatent.c > +++ b/fs/fat/fatent.c > @@ -682,6 +682,7 @@ int fat_count_free_clusters(struct super_block *sb) > if (ops->ent_get() == FAT_ENT_FREE) > free++; > } while (fat_ent_next(sbi, )); > + cond_resched(); > } > sbi->free_clusters = free; > sbi->free_clus_valid = 1; Acked-by: OGAWA Hirofumi Thanks, looks good. -- OGAWA Hirofumi
Re: [PATCH] fat: Expand a slightly out-of-date comment
Mihir Mehta writes: > The file namei.c seems to have been renamed to namei_msdos.c, so I > decided to update the comment with the correct name, and expand it a bit > to tell the reader what to look for. > --- > fs/fat/dir.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/fs/fat/dir.c b/fs/fat/dir.c > index 7f5f369..f4db13f 100644 > --- a/fs/fat/dir.c > +++ b/fs/fat/dir.c > @@ -369,7 +369,9 @@ static int fat_parse_short(struct super_block *sb, > } > > memcpy(work, de->name, sizeof(work)); > - /* see namei.c, msdos_format_name */ > + /* For an explanation of the special treatment of 0x05 in > + * filenames, see msdos_format_name in namei_msdos.c > + */ > if (work[0] == 0x05) > work[0] = 0xE5; Acked-by: OGAWA Hirofumi Thanks. -- OGAWA Hirofumi
Re: [PATCH] fat: Expand a slightly out-of-date comment
Mihir Mehta writes: > The file namei.c seems to have been renamed to namei_msdos.c, so I > decided to update the comment with the correct name, and expand it a bit > to tell the reader what to look for. > --- > fs/fat/dir.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/fs/fat/dir.c b/fs/fat/dir.c > index 7f5f369..f4db13f 100644 > --- a/fs/fat/dir.c > +++ b/fs/fat/dir.c > @@ -369,7 +369,9 @@ static int fat_parse_short(struct super_block *sb, > } > > memcpy(work, de->name, sizeof(work)); > - /* see namei.c, msdos_format_name */ > + /* For an explanation of the special treatment of 0x05 in > + * filenames, see msdos_format_name in namei_msdos.c > + */ > if (work[0] == 0x05) > work[0] = 0xE5; Acked-by: OGAWA Hirofumi Thanks. -- OGAWA Hirofumi
Re: [PATCH] fat: Expand a slightly out-of-date comment
Mihir Mehta writes:
> The file namei.c seems to have been renamed to namei_msdos.c, so I
> decided to update the comment with the correct name, and expand it a bit
> to tell the reader what to look for.
> ---
> fs/fat/dir.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/fs/fat/dir.c b/fs/fat/dir.c
> index b833ffe..d954e18 100644
> --- a/fs/fat/dir.c
> +++ b/fs/fat/dir.c
> @@ -368,7 +368,8 @@ static int fat_parse_short(struct super_block *sb,
> }
>
> memcpy(work, de->name, sizeof(work));
> - /* see namei.c, msdos_format_name */
> + /* For an explanation of the special treatment of 0x05 in
> +filenames, see msdos_format_name in namei_msdos.c */
> if (work[0] == 0x05)
> work[0] = 0xE5;
Sorry. However, could you use
/*
*
*/
style comment? Otherwise, looks good.
Thanks.
--
OGAWA Hirofumi
Re: [PATCH] fat: Expand a slightly out-of-date comment
Mihir Mehta writes:
> The file namei.c seems to have been renamed to namei_msdos.c, so I
> decided to update the comment with the correct name, and expand it a bit
> to tell the reader what to look for.
> ---
> fs/fat/dir.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/fs/fat/dir.c b/fs/fat/dir.c
> index b833ffe..d954e18 100644
> --- a/fs/fat/dir.c
> +++ b/fs/fat/dir.c
> @@ -368,7 +368,8 @@ static int fat_parse_short(struct super_block *sb,
> }
>
> memcpy(work, de->name, sizeof(work));
> - /* see namei.c, msdos_format_name */
> + /* For an explanation of the special treatment of 0x05 in
> +filenames, see msdos_format_name in namei_msdos.c */
> if (work[0] == 0x05)
> work[0] = 0xE5;
Sorry. However, could you use
/*
*
*/
style comment? Otherwise, looks good.
Thanks.
--
OGAWA Hirofumi
Re: [PATCH] fat: Relax checks for sector size and media type
Pali Rohár writes: >> If there is real user to use that, I'm ok though (of course, need >> serious tests). However, FAT would be for exchange data with other >> devices, and there is "cluster per sector", and spec recommends sector >> size == device sector size. So I suspect this format is not useful. > > I looked into OpenBSD, FreeBSD and NetBSD source code and there is no > explicit upper limit for sector size. Just that sector size must be > power of two. > > I have not did tests yet, but you are right that some testing should be > done. > > As FAT operates with clusters and cluster size is defined by sector > size, then sectors per cluster and sector size defines cluster size. And > cluster size itself implies maximal size of FAT filesystem. > > So increasing sector size could be useful to create larger FAT32 > filesystems as current limit hit by sector size = 512 bytes. > > What do you think, which operating systems should be tested? Again, I suspect those custom extension (can't read by some uefi or windows) is not useful though. Testing on kernel that has PAGE_SIZE >= 8192, and setting FAT sector_size >= 8192. After that, it would be safe to remove 4096 limitation. Thanks. -- OGAWA Hirofumi
Re: [PATCH] fat: Relax checks for sector size and media type
Pali Rohár writes: >> If there is real user to use that, I'm ok though (of course, need >> serious tests). However, FAT would be for exchange data with other >> devices, and there is "cluster per sector", and spec recommends sector >> size == device sector size. So I suspect this format is not useful. > > I looked into OpenBSD, FreeBSD and NetBSD source code and there is no > explicit upper limit for sector size. Just that sector size must be > power of two. > > I have not did tests yet, but you are right that some testing should be > done. > > As FAT operates with clusters and cluster size is defined by sector > size, then sectors per cluster and sector size defines cluster size. And > cluster size itself implies maximal size of FAT filesystem. > > So increasing sector size could be useful to create larger FAT32 > filesystems as current limit hit by sector size = 512 bytes. > > What do you think, which operating systems should be tested? Again, I suspect those custom extension (can't read by some uefi or windows) is not useful though. Testing on kernel that has PAGE_SIZE >= 8192, and setting FAT sector_size >= 8192. After that, it would be safe to remove 4096 limitation. Thanks. -- OGAWA Hirofumi
Re: [PATCH] fat: Relax checks for sector size and media type
Pali Rohár writes: >> That source seems to check power_of_2(size) and 128 <= size <= >> 4096. Rather why do you want to support larger than 4096? Or I'm missing >> something? > > I looked into (Linux) mkfs.fat and it supports formatting disk also with > sector size > 4096. Therefore I thought it may be good idea for ability > to mount and use it (on Linux). > > I could check what other operating system would do with FAT sector size > larger then 4096. If there is real user to use that, I'm ok though (of course, need serious tests). However, FAT would be for exchange data with other devices, and there is "cluster per sector", and spec recommends sector size == device sector size. So I suspect this format is not useful. Thanks. -- OGAWA Hirofumi
Re: [PATCH] fat: Relax checks for sector size and media type
Pali Rohár writes: >> That source seems to check power_of_2(size) and 128 <= size <= >> 4096. Rather why do you want to support larger than 4096? Or I'm missing >> something? > > I looked into (Linux) mkfs.fat and it supports formatting disk also with > sector size > 4096. Therefore I thought it may be good idea for ability > to mount and use it (on Linux). > > I could check what other operating system would do with FAT sector size > larger then 4096. If there is real user to use that, I'm ok though (of course, need serious tests). However, FAT would be for exchange data with other devices, and there is "cluster per sector", and spec recommends sector size == device sector size. So I suspect this format is not useful. Thanks. -- OGAWA Hirofumi
Re: [PATCH] fat: Relax checks for sector size and media type
Pali Rohár writes: >> Just relaxing validation doesn't work. The block layer doesn't support >> smaller than 512, and lager than PAGE_SIZE. (And in specification, fat >> doesn't support lager than 4096.) > > Hi! I just sent this patch for discussion, with links to (now open > source) Windows implementation. I guess that Windows driver > implementation is more "authoritative" then Microsoft's own > specification. It is known that Windows implementation does not match > Microsoft specification. > > I know at least 3 FAT specifications (MS EFI FAT, MS/SD card FAT, > ECMA-107) and you are right that Microsoft's one does not allow sector > sizes larger then 4096. > > If there is limitation by block layer, then: > > 1) Why we do not check for PAGE_SIZE? That source seems to check power_of_2(size) and 128 <= size <= 4096. Rather why do you want to support larger than 4096? Or I'm missing something? > 2) Is check in fat driver really needed (if block layer checks it)? Yes, isolating block layer error and fat format error to be better error report. -- OGAWA Hirofumi
Re: [PATCH] fat: Relax checks for sector size and media type
Pali Rohár writes: >> Just relaxing validation doesn't work. The block layer doesn't support >> smaller than 512, and lager than PAGE_SIZE. (And in specification, fat >> doesn't support lager than 4096.) > > Hi! I just sent this patch for discussion, with links to (now open > source) Windows implementation. I guess that Windows driver > implementation is more "authoritative" then Microsoft's own > specification. It is known that Windows implementation does not match > Microsoft specification. > > I know at least 3 FAT specifications (MS EFI FAT, MS/SD card FAT, > ECMA-107) and you are right that Microsoft's one does not allow sector > sizes larger then 4096. > > If there is limitation by block layer, then: > > 1) Why we do not check for PAGE_SIZE? That source seems to check power_of_2(size) and 128 <= size <= 4096. Rather why do you want to support larger than 4096? Or I'm missing something? > 2) Is check in fat driver really needed (if block layer checks it)? Yes, isolating block layer error and fat format error to be better error report. -- OGAWA Hirofumi
Re: [PATCH] fat: Relax checks for sector size and media type
Pali Rohár writes:
> Windows fastfat.sys driver accepts also media types 0x00 and 0x01 and
> sector sizes 128 and 256 bytes. Linux mkfs.fat can format disk also to
> larger FAT sector sizes then 4096 bytes, therefore relax also upper limit
> restriction.
> - if (!is_power_of_2(bpb->fat_sector_size)
> - || (bpb->fat_sector_size < 512)
> - || (bpb->fat_sector_size > 4096)) {
> + if (!is_power_of_2(bpb->fat_sector_size)) {
Just relaxing validation doesn't work. The block layer doesn't support
smaller than 512, and lager than PAGE_SIZE. (And in specification, fat
doesn't support lager than 4096.)
> static inline int fat_valid_media(u8 media)
> {
> - return 0xf8 <= media || media == 0xf0;
> + return 0xf8 <= media || media == 0xf0 || media == 0x00 || media == 0x01;
> }
> #endif /* !_LINUX_MSDOS_FS_H */
This is ok though, this would be for ancient floppy media.
Thanks.
--
OGAWA Hirofumi
Re: [PATCH] fat: Relax checks for sector size and media type
Pali Rohár writes:
> Windows fastfat.sys driver accepts also media types 0x00 and 0x01 and
> sector sizes 128 and 256 bytes. Linux mkfs.fat can format disk also to
> larger FAT sector sizes then 4096 bytes, therefore relax also upper limit
> restriction.
> - if (!is_power_of_2(bpb->fat_sector_size)
> - || (bpb->fat_sector_size < 512)
> - || (bpb->fat_sector_size > 4096)) {
> + if (!is_power_of_2(bpb->fat_sector_size)) {
Just relaxing validation doesn't work. The block layer doesn't support
smaller than 512, and lager than PAGE_SIZE. (And in specification, fat
doesn't support lager than 4096.)
> static inline int fat_valid_media(u8 media)
> {
> - return 0xf8 <= media || media == 0xf0;
> + return 0xf8 <= media || media == 0xf0 || media == 0x00 || media == 0x01;
> }
> #endif /* !_LINUX_MSDOS_FS_H */
This is ok though, this would be for ancient floppy media.
Thanks.
--
OGAWA Hirofumi
Re: [PATCH] Cleanup "fat: propagate 64-bit inode timestamps" patch
Arnd Bergmann writes:
>> /* Linear day numbers of the respective 1sts in non-leap years. */
>> -static time64_t days_in_year[] = {
>> +static long days_in_year[] = {
>> /* Jan Feb Mar Apr May Jun Jul Aug Sep Oct Nov Dec */
>> 0, 0, 31, 59, 90, 120, 151, 181, 212, 243, 273, 304, 334, 0, 0,
>> 0,
>> };
>
> While this is correct, changing it back to a signed 'long' type seems
> rather arbitrary. I tried to pick a type that would be the same on 32-bit
> and 64-bit architectures, the other choice would have been 'u16',
> which saves a few bytes.
Right. However, "long long" on 32bit arch is not same. In this implement
"long long" works though, Some calculation needs libgcc helper (in
kernel, div64 stuff). I want to avoid it early than later, and I don't
care saving a few memory in here (I would not care if it was u16).
Thanks.
--
OGAWA Hirofumi
Re: [PATCH] Cleanup "fat: propagate 64-bit inode timestamps" patch
Arnd Bergmann writes:
>> /* Linear day numbers of the respective 1sts in non-leap years. */
>> -static time64_t days_in_year[] = {
>> +static long days_in_year[] = {
>> /* Jan Feb Mar Apr May Jun Jul Aug Sep Oct Nov Dec */
>> 0, 0, 31, 59, 90, 120, 151, 181, 212, 243, 273, 304, 334, 0, 0,
>> 0,
>> };
>
> While this is correct, changing it back to a signed 'long' type seems
> rather arbitrary. I tried to pick a type that would be the same on 32-bit
> and 64-bit architectures, the other choice would have been 'u16',
> which saves a few bytes.
Right. However, "long long" on 32bit arch is not same. In this implement
"long long" works though, Some calculation needs libgcc helper (in
kernel, div64 stuff). I want to avoid it early than later, and I don't
care saving a few memory in here (I would not care if it was u16).
Thanks.
--
OGAWA Hirofumi
[PATCH] Cleanup "fat: propagate 64-bit inode timestamps" patch
Hi,
Looks like I missed the email to read for a patch
(mmots/broken-out/fat-propagate-64-bit-inode-timestamps.patch). Well,
so FWIW,
Acked-by: OGAWA Hirofumi
And additionally cleanup patch here (this would be better to be folded
into his patch).
Thanks.
--
OGAWA Hirofumi
[PATCH] Cleanup "fat: propagate 64-bit inode timestamps" patch
- Remove useless temporary variable
- Remove needless long long
Signed-off-by: OGAWA Hirofumi
---
fs/fat/inode.c | 10 +++---
fs/fat/misc.c |7 +++
2 files changed, 6 insertions(+), 11 deletions(-)
diff -puN fs/fat/inode.c~fat-propagate-64-bit-inode-timestamps-cleanup
fs/fat/inode.c
--- linux/fs/fat/inode.c~fat-propagate-64-bit-inode-timestamps-cleanup
2018-08-18 09:23:27.990137305 +0900
+++ linux-hirofumi/fs/fat/inode.c 2018-08-18 09:32:52.629008507 +0900
@@ -509,7 +509,6 @@ static int fat_validate_dir(struct inode
int fat_fill_inode(struct inode *inode, struct msdos_dir_entry *de)
{
struct msdos_sb_info *sbi = MSDOS_SB(inode->i_sb);
- struct timespec64 ts;
int error;
MSDOS_I(inode)->i_pos = 0;
@@ -559,14 +558,11 @@ int fat_fill_inode(struct inode *inode,
inode->i_blocks = ((inode->i_size + (sbi->cluster_size - 1))
& ~((loff_t)sbi->cluster_size - 1)) >> 9;
- fat_time_fat2unix(sbi, , de->time, de->date, 0);
- inode->i_mtime = ts;
+ fat_time_fat2unix(sbi, >i_mtime, de->time, de->date, 0);
if (sbi->options.isvfat) {
- fat_time_fat2unix(sbi, , de->ctime,
+ fat_time_fat2unix(sbi, >i_ctime, de->ctime,
de->cdate, de->ctime_cs);
- inode->i_ctime = ts;
- fat_time_fat2unix(sbi, , 0, de->adate, 0);
- inode->i_atime = ts;
+ fat_time_fat2unix(sbi, >i_atime, 0, de->adate, 0);
} else
inode->i_ctime = inode->i_atime = inode->i_mtime;
diff -puN fs/fat/misc.c~fat-propagate-64-bit-inode-timestamps-cleanup
fs/fat/misc.c
--- linux/fs/fat/misc.c~fat-propagate-64-bit-inode-timestamps-cleanup
2018-08-18 09:23:27.992137301 +0900
+++ linux-hirofumi/fs/fat/misc.c2018-08-18 09:32:52.596008572 +0900
@@ -180,7 +180,7 @@ int fat_chain_add(struct inode *inode, i
#define IS_LEAP_YEAR(y)(!((y) & 3) && (y) != YEAR_2100)
/* Linear day numbers of the respective 1sts in non-leap years. */
-static time64_t days_in_year[] = {
+static long days_in_year[] = {
/* Jan Feb Mar Apr May Jun Jul Aug Sep Oct Nov Dec */
0, 0, 31, 59, 90, 120, 151, 181, 212, 243, 273, 304, 334, 0, 0, 0,
};
@@ -191,8 +191,7 @@ void fat_time_fat2unix(struct msdos_sb_i
{
u16 time = le16_to_cpu(__time), date = le16_to_cpu(__date);
time64_t second;
- long day, leap_day, month;
- long long year;
+ long day, leap_day, month, year;
year = date >> 9;
month = max(1, (date >> 5) & 0xf);
@@ -207,7 +206,7 @@ void fat_time_fat2unix(struct msdos_sb_i
second = (time & 0x1f) << 1;
second += ((time >> 5) & 0x3f) * SECS_PER_MIN;
second += (time >> 11) * SECS_PER_HOUR;
- second += (year * 365 + leap_day
+ second += (time64_t)(year * 365 + leap_day
+ days_in_year[month] + day
+ DAYS_DELTA) * SECS_PER_DAY;
_
[PATCH] Cleanup "fat: propagate 64-bit inode timestamps" patch
Hi,
Looks like I missed the email to read for a patch
(mmots/broken-out/fat-propagate-64-bit-inode-timestamps.patch). Well,
so FWIW,
Acked-by: OGAWA Hirofumi
And additionally cleanup patch here (this would be better to be folded
into his patch).
Thanks.
--
OGAWA Hirofumi
[PATCH] Cleanup "fat: propagate 64-bit inode timestamps" patch
- Remove useless temporary variable
- Remove needless long long
Signed-off-by: OGAWA Hirofumi
---
fs/fat/inode.c | 10 +++---
fs/fat/misc.c |7 +++
2 files changed, 6 insertions(+), 11 deletions(-)
diff -puN fs/fat/inode.c~fat-propagate-64-bit-inode-timestamps-cleanup
fs/fat/inode.c
--- linux/fs/fat/inode.c~fat-propagate-64-bit-inode-timestamps-cleanup
2018-08-18 09:23:27.990137305 +0900
+++ linux-hirofumi/fs/fat/inode.c 2018-08-18 09:32:52.629008507 +0900
@@ -509,7 +509,6 @@ static int fat_validate_dir(struct inode
int fat_fill_inode(struct inode *inode, struct msdos_dir_entry *de)
{
struct msdos_sb_info *sbi = MSDOS_SB(inode->i_sb);
- struct timespec64 ts;
int error;
MSDOS_I(inode)->i_pos = 0;
@@ -559,14 +558,11 @@ int fat_fill_inode(struct inode *inode,
inode->i_blocks = ((inode->i_size + (sbi->cluster_size - 1))
& ~((loff_t)sbi->cluster_size - 1)) >> 9;
- fat_time_fat2unix(sbi, , de->time, de->date, 0);
- inode->i_mtime = ts;
+ fat_time_fat2unix(sbi, >i_mtime, de->time, de->date, 0);
if (sbi->options.isvfat) {
- fat_time_fat2unix(sbi, , de->ctime,
+ fat_time_fat2unix(sbi, >i_ctime, de->ctime,
de->cdate, de->ctime_cs);
- inode->i_ctime = ts;
- fat_time_fat2unix(sbi, , 0, de->adate, 0);
- inode->i_atime = ts;
+ fat_time_fat2unix(sbi, >i_atime, 0, de->adate, 0);
} else
inode->i_ctime = inode->i_atime = inode->i_mtime;
diff -puN fs/fat/misc.c~fat-propagate-64-bit-inode-timestamps-cleanup
fs/fat/misc.c
--- linux/fs/fat/misc.c~fat-propagate-64-bit-inode-timestamps-cleanup
2018-08-18 09:23:27.992137301 +0900
+++ linux-hirofumi/fs/fat/misc.c2018-08-18 09:32:52.596008572 +0900
@@ -180,7 +180,7 @@ int fat_chain_add(struct inode *inode, i
#define IS_LEAP_YEAR(y)(!((y) & 3) && (y) != YEAR_2100)
/* Linear day numbers of the respective 1sts in non-leap years. */
-static time64_t days_in_year[] = {
+static long days_in_year[] = {
/* Jan Feb Mar Apr May Jun Jul Aug Sep Oct Nov Dec */
0, 0, 31, 59, 90, 120, 151, 181, 212, 243, 273, 304, 334, 0, 0, 0,
};
@@ -191,8 +191,7 @@ void fat_time_fat2unix(struct msdos_sb_i
{
u16 time = le16_to_cpu(__time), date = le16_to_cpu(__date);
time64_t second;
- long day, leap_day, month;
- long long year;
+ long day, leap_day, month, year;
year = date >> 9;
month = max(1, (date >> 5) & 0xf);
@@ -207,7 +206,7 @@ void fat_time_fat2unix(struct msdos_sb_i
second = (time & 0x1f) << 1;
second += ((time >> 5) & 0x3f) * SECS_PER_MIN;
second += (time >> 11) * SECS_PER_HOUR;
- second += (year * 365 + leap_day
+ second += (time64_t)(year * 365 + leap_day
+ days_in_year[month] + day
+ DAYS_DELTA) * SECS_PER_DAY;
_
[PATCH] fat: Support timespec64 for 2107
On disk format of FAT is supporting until the end of 2107 year.
So this uses timespec64 internally without any truncation (until 2107).
Signed-off-by: OGAWA Hirofumi
---
fs/fat/dir.c |2 +-
fs/fat/fat.h |6 +++---
fs/fat/inode.c | 20 ++--
fs/fat/misc.c|4 ++--
fs/fat/namei_msdos.c | 17 ++---
fs/fat/namei_vfat.c | 20 +++-
6 files changed, 25 insertions(+), 44 deletions(-)
diff -puN fs/fat/dir.c~fat-timespec64-cleanup fs/fat/dir.c
--- linux/fs/fat/dir.c~fat-timespec64-cleanup 2018-08-14 01:09:15.355075178
+0900
+++ linux-hirofumi/fs/fat/dir.c 2018-08-14 01:38:37.527642880 +0900
@@ -1130,7 +1130,7 @@ error:
return err;
}
-int fat_alloc_new_dir(struct inode *dir, struct timespec *ts)
+int fat_alloc_new_dir(struct inode *dir, struct timespec64 *ts)
{
struct super_block *sb = dir->i_sb;
struct msdos_sb_info *sbi = MSDOS_SB(sb);
diff -puN fs/fat/misc.c~fat-timespec64-cleanup fs/fat/misc.c
--- linux/fs/fat/misc.c~fat-timespec64-cleanup 2018-08-14 01:09:15.356075177
+0900
+++ linux-hirofumi/fs/fat/misc.c2018-08-14 01:38:37.580642805 +0900
@@ -186,7 +186,7 @@ static time_t days_in_year[] = {
};
/* Convert a FAT time/date pair to a UNIX date (seconds since 1 1 70). */
-void fat_time_fat2unix(struct msdos_sb_info *sbi, struct timespec *ts,
+void fat_time_fat2unix(struct msdos_sb_info *sbi, struct timespec64 *ts,
__le16 __time, __le16 __date, u8 time_cs)
{
u16 time = le16_to_cpu(__time), date = le16_to_cpu(__date);
@@ -225,7 +225,7 @@ void fat_time_fat2unix(struct msdos_sb_i
}
/* Convert linear UNIX date to a FAT time/date pair. */
-void fat_time_unix2fat(struct msdos_sb_info *sbi, struct timespec *ts,
+void fat_time_unix2fat(struct msdos_sb_info *sbi, struct timespec64 *ts,
__le16 *time, __le16 *date, u8 *time_cs)
{
struct tm tm;
diff -puN fs/fat/inode.c~fat-timespec64-cleanup fs/fat/inode.c
--- linux/fs/fat/inode.c~fat-timespec64-cleanup 2018-08-14 01:09:15.357075175
+0900
+++ linux-hirofumi/fs/fat/inode.c 2018-08-14 01:38:37.613642759 +0900
@@ -508,7 +508,6 @@ static int fat_validate_dir(struct inode
/* doesn't deal with root inode */
int fat_fill_inode(struct inode *inode, struct msdos_dir_entry *de)
{
- struct timespec ts;
struct msdos_sb_info *sbi = MSDOS_SB(inode->i_sb);
int error;
@@ -559,14 +558,11 @@ int fat_fill_inode(struct inode *inode,
inode->i_blocks = ((inode->i_size + (sbi->cluster_size - 1))
& ~((loff_t)sbi->cluster_size - 1)) >> 9;
- fat_time_fat2unix(sbi, , de->time, de->date, 0);
- inode->i_mtime = timespec_to_timespec64(ts);
+ fat_time_fat2unix(sbi, >i_mtime, de->time, de->date, 0);
if (sbi->options.isvfat) {
- fat_time_fat2unix(sbi, , de->ctime,
+ fat_time_fat2unix(sbi, >i_ctime, de->ctime,
de->cdate, de->ctime_cs);
- inode->i_ctime = timespec_to_timespec64(ts);
- fat_time_fat2unix(sbi, , 0, de->adate, 0);
- inode->i_atime = timespec_to_timespec64(ts);
+ fat_time_fat2unix(sbi, >i_atime, 0, de->adate, 0);
} else
inode->i_ctime = inode->i_atime = inode->i_mtime;
@@ -843,7 +839,6 @@ static int fat_statfs(struct dentry *den
static int __fat_write_inode(struct inode *inode, int wait)
{
- struct timespec ts;
struct super_block *sb = inode->i_sb;
struct msdos_sb_info *sbi = MSDOS_SB(sb);
struct buffer_head *bh;
@@ -881,16 +876,13 @@ retry:
raw_entry->size = cpu_to_le32(inode->i_size);
raw_entry->attr = fat_make_attrs(inode);
fat_set_start(raw_entry, MSDOS_I(inode)->i_logstart);
- ts = timespec64_to_timespec(inode->i_mtime);
- fat_time_unix2fat(sbi, , _entry->time,
+ fat_time_unix2fat(sbi, >i_mtime, _entry->time,
_entry->date, NULL);
if (sbi->options.isvfat) {
__le16 atime;
- ts = timespec64_to_timespec(inode->i_ctime);
- fat_time_unix2fat(sbi, , _entry->ctime,
+ fat_time_unix2fat(sbi, >i_ctime, _entry->ctime,
_entry->cdate, _entry->ctime_cs);
- ts = timespec64_to_timespec(inode->i_atime);
- fat_time_unix2fat(sbi, , ,
+ fat_time_unix2fat(sbi, >i_atime, ,
_entry->adate, NULL);
}
spin_unlock(>inode_hash_lock);
diff -puN fs/fat/namei_msdos.c~fat-timespec64-cleanup fs/fat/namei_msdos.c
--- linux/fs/fat/namei_msdos.c~fat-timespec64-cleanup 2018-08-14
01:09:15.358075174 +0900
+++ linux-hirofumi/fs/fat/namei_msdo
[PATCH] fat: Support timespec64 for 2107
On disk format of FAT is supporting until the end of 2107 year.
So this uses timespec64 internally without any truncation (until 2107).
Signed-off-by: OGAWA Hirofumi
---
fs/fat/dir.c |2 +-
fs/fat/fat.h |6 +++---
fs/fat/inode.c | 20 ++--
fs/fat/misc.c|4 ++--
fs/fat/namei_msdos.c | 17 ++---
fs/fat/namei_vfat.c | 20 +++-
6 files changed, 25 insertions(+), 44 deletions(-)
diff -puN fs/fat/dir.c~fat-timespec64-cleanup fs/fat/dir.c
--- linux/fs/fat/dir.c~fat-timespec64-cleanup 2018-08-14 01:09:15.355075178
+0900
+++ linux-hirofumi/fs/fat/dir.c 2018-08-14 01:38:37.527642880 +0900
@@ -1130,7 +1130,7 @@ error:
return err;
}
-int fat_alloc_new_dir(struct inode *dir, struct timespec *ts)
+int fat_alloc_new_dir(struct inode *dir, struct timespec64 *ts)
{
struct super_block *sb = dir->i_sb;
struct msdos_sb_info *sbi = MSDOS_SB(sb);
diff -puN fs/fat/misc.c~fat-timespec64-cleanup fs/fat/misc.c
--- linux/fs/fat/misc.c~fat-timespec64-cleanup 2018-08-14 01:09:15.356075177
+0900
+++ linux-hirofumi/fs/fat/misc.c2018-08-14 01:38:37.580642805 +0900
@@ -186,7 +186,7 @@ static time_t days_in_year[] = {
};
/* Convert a FAT time/date pair to a UNIX date (seconds since 1 1 70). */
-void fat_time_fat2unix(struct msdos_sb_info *sbi, struct timespec *ts,
+void fat_time_fat2unix(struct msdos_sb_info *sbi, struct timespec64 *ts,
__le16 __time, __le16 __date, u8 time_cs)
{
u16 time = le16_to_cpu(__time), date = le16_to_cpu(__date);
@@ -225,7 +225,7 @@ void fat_time_fat2unix(struct msdos_sb_i
}
/* Convert linear UNIX date to a FAT time/date pair. */
-void fat_time_unix2fat(struct msdos_sb_info *sbi, struct timespec *ts,
+void fat_time_unix2fat(struct msdos_sb_info *sbi, struct timespec64 *ts,
__le16 *time, __le16 *date, u8 *time_cs)
{
struct tm tm;
diff -puN fs/fat/inode.c~fat-timespec64-cleanup fs/fat/inode.c
--- linux/fs/fat/inode.c~fat-timespec64-cleanup 2018-08-14 01:09:15.357075175
+0900
+++ linux-hirofumi/fs/fat/inode.c 2018-08-14 01:38:37.613642759 +0900
@@ -508,7 +508,6 @@ static int fat_validate_dir(struct inode
/* doesn't deal with root inode */
int fat_fill_inode(struct inode *inode, struct msdos_dir_entry *de)
{
- struct timespec ts;
struct msdos_sb_info *sbi = MSDOS_SB(inode->i_sb);
int error;
@@ -559,14 +558,11 @@ int fat_fill_inode(struct inode *inode,
inode->i_blocks = ((inode->i_size + (sbi->cluster_size - 1))
& ~((loff_t)sbi->cluster_size - 1)) >> 9;
- fat_time_fat2unix(sbi, , de->time, de->date, 0);
- inode->i_mtime = timespec_to_timespec64(ts);
+ fat_time_fat2unix(sbi, >i_mtime, de->time, de->date, 0);
if (sbi->options.isvfat) {
- fat_time_fat2unix(sbi, , de->ctime,
+ fat_time_fat2unix(sbi, >i_ctime, de->ctime,
de->cdate, de->ctime_cs);
- inode->i_ctime = timespec_to_timespec64(ts);
- fat_time_fat2unix(sbi, , 0, de->adate, 0);
- inode->i_atime = timespec_to_timespec64(ts);
+ fat_time_fat2unix(sbi, >i_atime, 0, de->adate, 0);
} else
inode->i_ctime = inode->i_atime = inode->i_mtime;
@@ -843,7 +839,6 @@ static int fat_statfs(struct dentry *den
static int __fat_write_inode(struct inode *inode, int wait)
{
- struct timespec ts;
struct super_block *sb = inode->i_sb;
struct msdos_sb_info *sbi = MSDOS_SB(sb);
struct buffer_head *bh;
@@ -881,16 +876,13 @@ retry:
raw_entry->size = cpu_to_le32(inode->i_size);
raw_entry->attr = fat_make_attrs(inode);
fat_set_start(raw_entry, MSDOS_I(inode)->i_logstart);
- ts = timespec64_to_timespec(inode->i_mtime);
- fat_time_unix2fat(sbi, , _entry->time,
+ fat_time_unix2fat(sbi, >i_mtime, _entry->time,
_entry->date, NULL);
if (sbi->options.isvfat) {
__le16 atime;
- ts = timespec64_to_timespec(inode->i_ctime);
- fat_time_unix2fat(sbi, , _entry->ctime,
+ fat_time_unix2fat(sbi, >i_ctime, _entry->ctime,
_entry->cdate, _entry->ctime_cs);
- ts = timespec64_to_timespec(inode->i_atime);
- fat_time_unix2fat(sbi, , ,
+ fat_time_unix2fat(sbi, >i_atime, ,
_entry->adate, NULL);
}
spin_unlock(>inode_hash_lock);
diff -puN fs/fat/namei_msdos.c~fat-timespec64-cleanup fs/fat/namei_msdos.c
--- linux/fs/fat/namei_msdos.c~fat-timespec64-cleanup 2018-08-14
01:09:15.358075174 +0900
+++ linux-hirofumi/fs/fat/namei_msdo
Re: FAT: Operating on broken FAT FS causes the write syscall to return negative number not equal to -1
Anatoly Trosinenko writes: >> This patch returns better error (-EIO) for me. > > This works for me likewise. Thanks for testing. >> (But note, the corrupted FS image doesn't guarantee POSIX behavior.) > > Oops, I was just doing some testing and thought that correct behavior > for crafted FS is to return arbitrary valid error code (like -EIO) or > some arbitrary data, say, not larger than FS (not disclosing the > kernel memory, of course). Please excuse me if I was wrong. If fixing > this would slow down some hot code path, then I am not insisting on > returning valid errno. :) > > Meanwhile, how should be considered such discrepancies with man pages > for invalid FS images: should it be considered low priority bug, > not-a-bug or feature request (diagnostics)? To handle the corrupted image _perfectly_, finally we will have to have online fsck or similar. For example, if the data block was shared between the regular file and directory, user can mmap the directory data via (corrupted) regular file. Then locking of directory handler doesn't work, and handler has to have directory data as volatile data (and validate data for each memory load). And to verify this invalid shared data blocks, we will have to read all inodes (i.e. almost fsck). So, I may change the code if data verification is easy and lightweight like in this case. But like said above, it will not be guaranteed. Thanks. -- OGAWA Hirofumi
Re: FAT: Operating on broken FAT FS causes the write syscall to return negative number not equal to -1
Anatoly Trosinenko writes: >> This patch returns better error (-EIO) for me. > > This works for me likewise. Thanks for testing. >> (But note, the corrupted FS image doesn't guarantee POSIX behavior.) > > Oops, I was just doing some testing and thought that correct behavior > for crafted FS is to return arbitrary valid error code (like -EIO) or > some arbitrary data, say, not larger than FS (not disclosing the > kernel memory, of course). Please excuse me if I was wrong. If fixing > this would slow down some hot code path, then I am not insisting on > returning valid errno. :) > > Meanwhile, how should be considered such discrepancies with man pages > for invalid FS images: should it be considered low priority bug, > not-a-bug or feature request (diagnostics)? To handle the corrupted image _perfectly_, finally we will have to have online fsck or similar. For example, if the data block was shared between the regular file and directory, user can mmap the directory data via (corrupted) regular file. Then locking of directory handler doesn't work, and handler has to have directory data as volatile data (and validate data for each memory load). And to verify this invalid shared data blocks, we will have to read all inodes (i.e. almost fsck). So, I may change the code if data verification is easy and lightweight like in this case. But like said above, it will not be guaranteed. Thanks. -- OGAWA Hirofumi
[PATCH v2] Re: FAT: Operating on broken FAT FS causes the write syscall to return negative number not equal to -1
Al Viro writes:
> On Sun, Jul 15, 2018 at 11:20:06PM +0900, OGAWA Hirofumi wrote:
>> +static inline bool fat_valid_entry(struct msdos_sb_info *sbi, int entry)
>> +{
>> +if (entry < FAT_START_ENT || sbi->max_cluster <= entry)
>> +return false;
>> +return true;
>> +}
>
> Pet peeve: if (...) return false; return true; instead of return !;
>
> In this case,
> return entry >= FAT_START_ENT && entry < sb->max_cluster;
Fixed. Thanks.
[PATCH v2] fat: Validate ->i_start before using
On corrupted FATfs may have invalid ->i_start. To handle it, this
checks ->i_start before using, and return proper error code.
Signed-off-by: OGAWA Hirofumi
---
fs/fat/cache.c | 19 ---
fs/fat/fat.h|5 +
fs/fat/fatent.c |6 +++---
3 files changed, 20 insertions(+), 10 deletions(-)
diff -puN fs/fat/cache.c~fat-validate-i_start fs/fat/cache.c
--- linux/fs/fat/cache.c~fat-validate-i_start 2018-07-15 23:03:25.167171670
+0900
+++ linux-hirofumi/fs/fat/cache.c 2018-07-15 23:59:11.978489716 +0900
@@ -225,7 +225,8 @@ static inline void cache_init(struct fat
int fat_get_cluster(struct inode *inode, int cluster, int *fclus, int *dclus)
{
struct super_block *sb = inode->i_sb;
- const int limit = sb->s_maxbytes >> MSDOS_SB(sb)->cluster_bits;
+ struct msdos_sb_info *sbi = MSDOS_SB(sb);
+ const int limit = sb->s_maxbytes >> sbi->cluster_bits;
struct fat_entry fatent;
struct fat_cache_id cid;
int nr;
@@ -234,6 +235,12 @@ int fat_get_cluster(struct inode *inode,
*fclus = 0;
*dclus = MSDOS_I(inode)->i_start;
+ if (!fat_valid_entry(sbi, *dclus)) {
+ fat_fs_error_ratelimit(sb,
+ "%s: invalid start cluster (i_pos %lld, start %08x)",
+ __func__, MSDOS_I(inode)->i_pos, *dclus);
+ return -EIO;
+ }
if (cluster == 0)
return 0;
@@ -250,9 +257,8 @@ int fat_get_cluster(struct inode *inode,
/* prevent the infinite loop of cluster chain */
if (*fclus > limit) {
fat_fs_error_ratelimit(sb,
- "%s: detected the cluster chain loop"
- " (i_pos %lld)", __func__,
- MSDOS_I(inode)->i_pos);
+ "%s: detected the cluster chain loop (i_pos
%lld)",
+ __func__, MSDOS_I(inode)->i_pos);
nr = -EIO;
goto out;
}
@@ -262,9 +268,8 @@ int fat_get_cluster(struct inode *inode,
goto out;
else if (nr == FAT_ENT_FREE) {
fat_fs_error_ratelimit(sb,
- "%s: invalid cluster chain (i_pos %lld)",
- __func__,
- MSDOS_I(inode)->i_pos);
+ "%s: invalid cluster chain (i_pos %lld)",
+ __func__, MSDOS_I(inode)->i_pos);
nr = -EIO;
goto out;
} else if (nr == FAT_ENT_EOF) {
diff -puN fs/fat/fat.h~fat-validate-i_start fs/fat/fat.h
--- linux/fs/fat/fat.h~fat-validate-i_start 2018-07-15 23:03:25.168171670
+0900
+++ linux-hirofumi/fs/fat/fat.h 2018-07-15 23:59:58.896437024 +0900
@@ -348,6 +348,11 @@ static inline void fatent_brelse(struct
fatent->fat_inode = NULL;
}
+static inline bool fat_valid_entry(struct msdos_sb_info *sbi, int entry)
+{
+ return FAT_START_ENT <= entry && entry < sbi->max_cluster;
+}
+
extern void fat_ent_access_init(struct super_block *sb);
extern int fat_ent_read(struct inode *inode, struct fat_entry *fatent,
int entry);
diff -puN fs/fat/fatent.c~fat-validate-i_start fs/fat/fatent.c
--- linux/fs/fat/fatent.c~fat-validate-i_start 2018-07-15 23:03:25.169171668
+0900
+++ linux-hirofumi/fs/fat/fatent.c 2018-07-15 23:59:12.036489650 +0900
@@ -24,7 +24,7 @@ static void fat12_ent_blocknr(struct sup
{
struct msdos_sb_info *sbi = MSDOS_SB(sb);
int bytes = entry + (entry >> 1);
- WARN_ON(entry < FAT_START_ENT || sbi->max_cluster <= entry);
+ WARN_ON(!fat_valid_entry(sbi, entry));
*offset = bytes & (sb->s_blocksize - 1);
*blocknr = sbi->fat_start + (bytes >> sb->s_blocksize_bits);
}
@@ -34,7 +34,7 @@ static void fat_ent_blocknr(struct super
{
struct msdos_sb_info *sbi = MSDOS_SB(sb);
int bytes = (entry << sbi->fatent_shift);
- WARN_ON(entry < FAT_START_ENT || sbi->max_clust
[PATCH v2] Re: FAT: Operating on broken FAT FS causes the write syscall to return negative number not equal to -1
Al Viro writes:
> On Sun, Jul 15, 2018 at 11:20:06PM +0900, OGAWA Hirofumi wrote:
>> +static inline bool fat_valid_entry(struct msdos_sb_info *sbi, int entry)
>> +{
>> +if (entry < FAT_START_ENT || sbi->max_cluster <= entry)
>> +return false;
>> +return true;
>> +}
>
> Pet peeve: if (...) return false; return true; instead of return !;
>
> In this case,
> return entry >= FAT_START_ENT && entry < sb->max_cluster;
Fixed. Thanks.
[PATCH v2] fat: Validate ->i_start before using
On corrupted FATfs may have invalid ->i_start. To handle it, this
checks ->i_start before using, and return proper error code.
Signed-off-by: OGAWA Hirofumi
---
fs/fat/cache.c | 19 ---
fs/fat/fat.h|5 +
fs/fat/fatent.c |6 +++---
3 files changed, 20 insertions(+), 10 deletions(-)
diff -puN fs/fat/cache.c~fat-validate-i_start fs/fat/cache.c
--- linux/fs/fat/cache.c~fat-validate-i_start 2018-07-15 23:03:25.167171670
+0900
+++ linux-hirofumi/fs/fat/cache.c 2018-07-15 23:59:11.978489716 +0900
@@ -225,7 +225,8 @@ static inline void cache_init(struct fat
int fat_get_cluster(struct inode *inode, int cluster, int *fclus, int *dclus)
{
struct super_block *sb = inode->i_sb;
- const int limit = sb->s_maxbytes >> MSDOS_SB(sb)->cluster_bits;
+ struct msdos_sb_info *sbi = MSDOS_SB(sb);
+ const int limit = sb->s_maxbytes >> sbi->cluster_bits;
struct fat_entry fatent;
struct fat_cache_id cid;
int nr;
@@ -234,6 +235,12 @@ int fat_get_cluster(struct inode *inode,
*fclus = 0;
*dclus = MSDOS_I(inode)->i_start;
+ if (!fat_valid_entry(sbi, *dclus)) {
+ fat_fs_error_ratelimit(sb,
+ "%s: invalid start cluster (i_pos %lld, start %08x)",
+ __func__, MSDOS_I(inode)->i_pos, *dclus);
+ return -EIO;
+ }
if (cluster == 0)
return 0;
@@ -250,9 +257,8 @@ int fat_get_cluster(struct inode *inode,
/* prevent the infinite loop of cluster chain */
if (*fclus > limit) {
fat_fs_error_ratelimit(sb,
- "%s: detected the cluster chain loop"
- " (i_pos %lld)", __func__,
- MSDOS_I(inode)->i_pos);
+ "%s: detected the cluster chain loop (i_pos
%lld)",
+ __func__, MSDOS_I(inode)->i_pos);
nr = -EIO;
goto out;
}
@@ -262,9 +268,8 @@ int fat_get_cluster(struct inode *inode,
goto out;
else if (nr == FAT_ENT_FREE) {
fat_fs_error_ratelimit(sb,
- "%s: invalid cluster chain (i_pos %lld)",
- __func__,
- MSDOS_I(inode)->i_pos);
+ "%s: invalid cluster chain (i_pos %lld)",
+ __func__, MSDOS_I(inode)->i_pos);
nr = -EIO;
goto out;
} else if (nr == FAT_ENT_EOF) {
diff -puN fs/fat/fat.h~fat-validate-i_start fs/fat/fat.h
--- linux/fs/fat/fat.h~fat-validate-i_start 2018-07-15 23:03:25.168171670
+0900
+++ linux-hirofumi/fs/fat/fat.h 2018-07-15 23:59:58.896437024 +0900
@@ -348,6 +348,11 @@ static inline void fatent_brelse(struct
fatent->fat_inode = NULL;
}
+static inline bool fat_valid_entry(struct msdos_sb_info *sbi, int entry)
+{
+ return FAT_START_ENT <= entry && entry < sbi->max_cluster;
+}
+
extern void fat_ent_access_init(struct super_block *sb);
extern int fat_ent_read(struct inode *inode, struct fat_entry *fatent,
int entry);
diff -puN fs/fat/fatent.c~fat-validate-i_start fs/fat/fatent.c
--- linux/fs/fat/fatent.c~fat-validate-i_start 2018-07-15 23:03:25.169171668
+0900
+++ linux-hirofumi/fs/fat/fatent.c 2018-07-15 23:59:12.036489650 +0900
@@ -24,7 +24,7 @@ static void fat12_ent_blocknr(struct sup
{
struct msdos_sb_info *sbi = MSDOS_SB(sb);
int bytes = entry + (entry >> 1);
- WARN_ON(entry < FAT_START_ENT || sbi->max_cluster <= entry);
+ WARN_ON(!fat_valid_entry(sbi, entry));
*offset = bytes & (sb->s_blocksize - 1);
*blocknr = sbi->fat_start + (bytes >> sb->s_blocksize_bits);
}
@@ -34,7 +34,7 @@ static void fat_ent_blocknr(struct super
{
struct msdos_sb_info *sbi = MSDOS_SB(sb);
int bytes = (entry << sbi->fatent_shift);
- WARN_ON(entry < FAT_START_ENT || sbi->max_clust
Re: FAT: Operating on broken FAT FS causes the write syscall to return negative number not equal to -1
Anatoly Trosinenko writes:
> How to reproduce:
> 1) Compile v4.18-rc4 kernel with the attached config
> 1) Unpack the attached FS image (128 Mb) and mount it as vfat to /mnt
> 2) Compile and run vfat-bug.c
>
> What is expected:
> `write` returns either -1 or small positive number.
>
> What happens:
> The -13619152 aka 0xff303030 is returned.
This patch returns better error (-EIO) for me. (But note, the corrupted
FS image doesn't guarantee POSIX behavior.)
Thanks.
[PATCH] fat: Validate ->i_start before using
On corrupted FATfs may have invalid ->i_start. To handle it, this
checks ->i_start before using, and return proper error code.
Signed-off-by: OGAWA Hirofumi
---
fs/fat/cache.c | 19 ---
fs/fat/fat.h|7 +++
fs/fat/fatent.c |6 +++---
3 files changed, 22 insertions(+), 10 deletions(-)
diff -puN fs/fat/cache.c~fat-validate-i_start fs/fat/cache.c
--- linux/fs/fat/cache.c~fat-validate-i_start 2018-07-15 23:03:25.167171670
+0900
+++ linux-hirofumi/fs/fat/cache.c 2018-07-15 23:03:25.171171666 +0900
@@ -225,7 +225,8 @@ static inline void cache_init(struct fat
int fat_get_cluster(struct inode *inode, int cluster, int *fclus, int *dclus)
{
struct super_block *sb = inode->i_sb;
- const int limit = sb->s_maxbytes >> MSDOS_SB(sb)->cluster_bits;
+ struct msdos_sb_info *sbi = MSDOS_SB(sb);
+ const int limit = sb->s_maxbytes >> sbi->cluster_bits;
struct fat_entry fatent;
struct fat_cache_id cid;
int nr;
@@ -234,6 +235,12 @@ int fat_get_cluster(struct inode *inode,
*fclus = 0;
*dclus = MSDOS_I(inode)->i_start;
+ if (!fat_valid_entry(sbi, *dclus)) {
+ fat_fs_error_ratelimit(sb,
+ "%s: invalid start cluster (i_pos %lld, start %08x)",
+ __func__, MSDOS_I(inode)->i_pos, *dclus);
+ return -EIO;
+ }
if (cluster == 0)
return 0;
@@ -250,9 +257,8 @@ int fat_get_cluster(struct inode *inode,
/* prevent the infinite loop of cluster chain */
if (*fclus > limit) {
fat_fs_error_ratelimit(sb,
- "%s: detected the cluster chain loop"
- " (i_pos %lld)", __func__,
- MSDOS_I(inode)->i_pos);
+ "%s: detected the cluster chain loop (i_pos
%lld)",
+ __func__, MSDOS_I(inode)->i_pos);
nr = -EIO;
goto out;
}
@@ -262,9 +268,8 @@ int fat_get_cluster(struct inode *inode,
goto out;
else if (nr == FAT_ENT_FREE) {
fat_fs_error_ratelimit(sb,
- "%s: invalid cluster chain (i_pos %lld)",
- __func__,
- MSDOS_I(inode)->i_pos);
+ "%s: invalid cluster chain (i_pos %lld)",
+ __func__, MSDOS_I(inode)->i_pos);
nr = -EIO;
goto out;
} else if (nr == FAT_ENT_EOF) {
diff -puN fs/fat/fat.h~fat-validate-i_start fs/fat/fat.h
--- linux/fs/fat/fat.h~fat-validate-i_start 2018-07-15 23:03:25.168171670
+0900
+++ linux-hirofumi/fs/fat/fat.h 2018-07-15 23:03:25.171171666 +0900
@@ -348,6 +348,13 @@ static inline void fatent_brelse(struct
fatent->fat_inode = NULL;
}
+static inline bool fat_valid_entry(struct msdos_sb_info *sbi, int entry)
+{
+ if (entry < FAT_START_ENT || sbi->max_cluster <= entry)
+ return false;
+ return true;
+}
+
extern void fat_ent_access_init(struct super_block *sb);
extern int fat_ent_read(struct inode *inode, struct fat_entry *fatent,
int entry);
diff -puN fs/fat/fatent.c~fat-validate-i_start fs/fat/fatent.c
--- linux/fs/fat/fatent.c~fat-validate-i_start 2018-07-15 23:03:25.169171668
+0900
+++ linux-hirofumi/fs/fat/fatent.c 2018-07-15 23:03:25.171171666 +0900
@@ -24,7 +24,7 @@ static void fat12_ent_blocknr(struct sup
{
struct msdos_sb_info *sbi = MSDOS_SB(sb);
int bytes = entry + (entry >> 1);
- WARN_ON(entry < FAT_START_ENT || sbi->max_cluster <= entry);
+ WARN_ON(!fat_valid_entry(sbi, entry));
*offset = bytes & (sb->s_blocksize - 1);
*blocknr = sbi->fat_start + (bytes >> sb->s_blocksize_bits);
}
@@ -34,7 +34,7 @@ static void fat_ent_blocknr(struct super
{
struct msdos_sb_info *sbi = MSDOS_SB(sb);
int bytes = (entry << sbi->fatent_shift);
- WARN_ON(entry < FAT_START_ENT || sbi->max_cl
Re: FAT: Operating on broken FAT FS causes the write syscall to return negative number not equal to -1
Anatoly Trosinenko writes:
> How to reproduce:
> 1) Compile v4.18-rc4 kernel with the attached config
> 1) Unpack the attached FS image (128 Mb) and mount it as vfat to /mnt
> 2) Compile and run vfat-bug.c
>
> What is expected:
> `write` returns either -1 or small positive number.
>
> What happens:
> The -13619152 aka 0xff303030 is returned.
This patch returns better error (-EIO) for me. (But note, the corrupted
FS image doesn't guarantee POSIX behavior.)
Thanks.
[PATCH] fat: Validate ->i_start before using
On corrupted FATfs may have invalid ->i_start. To handle it, this
checks ->i_start before using, and return proper error code.
Signed-off-by: OGAWA Hirofumi
---
fs/fat/cache.c | 19 ---
fs/fat/fat.h|7 +++
fs/fat/fatent.c |6 +++---
3 files changed, 22 insertions(+), 10 deletions(-)
diff -puN fs/fat/cache.c~fat-validate-i_start fs/fat/cache.c
--- linux/fs/fat/cache.c~fat-validate-i_start 2018-07-15 23:03:25.167171670
+0900
+++ linux-hirofumi/fs/fat/cache.c 2018-07-15 23:03:25.171171666 +0900
@@ -225,7 +225,8 @@ static inline void cache_init(struct fat
int fat_get_cluster(struct inode *inode, int cluster, int *fclus, int *dclus)
{
struct super_block *sb = inode->i_sb;
- const int limit = sb->s_maxbytes >> MSDOS_SB(sb)->cluster_bits;
+ struct msdos_sb_info *sbi = MSDOS_SB(sb);
+ const int limit = sb->s_maxbytes >> sbi->cluster_bits;
struct fat_entry fatent;
struct fat_cache_id cid;
int nr;
@@ -234,6 +235,12 @@ int fat_get_cluster(struct inode *inode,
*fclus = 0;
*dclus = MSDOS_I(inode)->i_start;
+ if (!fat_valid_entry(sbi, *dclus)) {
+ fat_fs_error_ratelimit(sb,
+ "%s: invalid start cluster (i_pos %lld, start %08x)",
+ __func__, MSDOS_I(inode)->i_pos, *dclus);
+ return -EIO;
+ }
if (cluster == 0)
return 0;
@@ -250,9 +257,8 @@ int fat_get_cluster(struct inode *inode,
/* prevent the infinite loop of cluster chain */
if (*fclus > limit) {
fat_fs_error_ratelimit(sb,
- "%s: detected the cluster chain loop"
- " (i_pos %lld)", __func__,
- MSDOS_I(inode)->i_pos);
+ "%s: detected the cluster chain loop (i_pos
%lld)",
+ __func__, MSDOS_I(inode)->i_pos);
nr = -EIO;
goto out;
}
@@ -262,9 +268,8 @@ int fat_get_cluster(struct inode *inode,
goto out;
else if (nr == FAT_ENT_FREE) {
fat_fs_error_ratelimit(sb,
- "%s: invalid cluster chain (i_pos %lld)",
- __func__,
- MSDOS_I(inode)->i_pos);
+ "%s: invalid cluster chain (i_pos %lld)",
+ __func__, MSDOS_I(inode)->i_pos);
nr = -EIO;
goto out;
} else if (nr == FAT_ENT_EOF) {
diff -puN fs/fat/fat.h~fat-validate-i_start fs/fat/fat.h
--- linux/fs/fat/fat.h~fat-validate-i_start 2018-07-15 23:03:25.168171670
+0900
+++ linux-hirofumi/fs/fat/fat.h 2018-07-15 23:03:25.171171666 +0900
@@ -348,6 +348,13 @@ static inline void fatent_brelse(struct
fatent->fat_inode = NULL;
}
+static inline bool fat_valid_entry(struct msdos_sb_info *sbi, int entry)
+{
+ if (entry < FAT_START_ENT || sbi->max_cluster <= entry)
+ return false;
+ return true;
+}
+
extern void fat_ent_access_init(struct super_block *sb);
extern int fat_ent_read(struct inode *inode, struct fat_entry *fatent,
int entry);
diff -puN fs/fat/fatent.c~fat-validate-i_start fs/fat/fatent.c
--- linux/fs/fat/fatent.c~fat-validate-i_start 2018-07-15 23:03:25.169171668
+0900
+++ linux-hirofumi/fs/fat/fatent.c 2018-07-15 23:03:25.171171666 +0900
@@ -24,7 +24,7 @@ static void fat12_ent_blocknr(struct sup
{
struct msdos_sb_info *sbi = MSDOS_SB(sb);
int bytes = entry + (entry >> 1);
- WARN_ON(entry < FAT_START_ENT || sbi->max_cluster <= entry);
+ WARN_ON(!fat_valid_entry(sbi, entry));
*offset = bytes & (sb->s_blocksize - 1);
*blocknr = sbi->fat_start + (bytes >> sb->s_blocksize_bits);
}
@@ -34,7 +34,7 @@ static void fat_ent_blocknr(struct super
{
struct msdos_sb_info *sbi = MSDOS_SB(sb);
int bytes = (entry << sbi->fatent_shift);
- WARN_ON(entry < FAT_START_ENT || sbi->max_cl
[PATCH] fat: Fix potential shift wrap with FITRIM ioctl on FAT
This patch is the fix of fat-add-fitrim-ioctl-for-fat-file-system.patch. Maybe better to merge with it (if it is easy). Anyway, please apply this with above patch. From: Wentao Wang If we keep "trimmed" as an u32, there will be a potential shift wrap. It would be a problem on a larger than 4GB partition with FAT32. Though most tools who call this ioctl would ignore this value, it would be great to fix it. Signed-off-by: Wentao Wang Signed-off-by: OGAWA Hirofumi --- fs/fat/fatent.c |4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff -puN fs/fat/fatent.c~fat-fitrim-fix fs/fat/fatent.c --- linux/fs/fat/fatent.c~fat-fitrim-fix2018-07-13 15:39:14.417110998 +0900 +++ linux-hirofumi/fs/fat/fatent.c 2018-07-13 15:39:14.418110996 +0900 @@ -705,8 +705,8 @@ int fat_trim_fs(struct inode *inode, str struct msdos_sb_info *sbi = MSDOS_SB(sb); const struct fatent_operations *ops = sbi->fatent_ops; struct fat_entry fatent; - u64 ent_start, ent_end, minlen; - u32 free = 0, trimmed = 0; + u64 ent_start, ent_end, minlen, trimmed = 0; + u32 free = 0; unsigned long reada_blocks, reada_mask, cur_block = 0; int err = 0; _ -- OGAWA Hirofumi
[PATCH] fat: Fix potential shift wrap with FITRIM ioctl on FAT
This patch is the fix of fat-add-fitrim-ioctl-for-fat-file-system.patch. Maybe better to merge with it (if it is easy). Anyway, please apply this with above patch. From: Wentao Wang If we keep "trimmed" as an u32, there will be a potential shift wrap. It would be a problem on a larger than 4GB partition with FAT32. Though most tools who call this ioctl would ignore this value, it would be great to fix it. Signed-off-by: Wentao Wang Signed-off-by: OGAWA Hirofumi --- fs/fat/fatent.c |4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff -puN fs/fat/fatent.c~fat-fitrim-fix fs/fat/fatent.c --- linux/fs/fat/fatent.c~fat-fitrim-fix2018-07-13 15:39:14.417110998 +0900 +++ linux-hirofumi/fs/fat/fatent.c 2018-07-13 15:39:14.418110996 +0900 @@ -705,8 +705,8 @@ int fat_trim_fs(struct inode *inode, str struct msdos_sb_info *sbi = MSDOS_SB(sb); const struct fatent_operations *ops = sbi->fatent_ops; struct fat_entry fatent; - u64 ent_start, ent_end, minlen; - u32 free = 0, trimmed = 0; + u64 ent_start, ent_end, minlen, trimmed = 0; + u32 free = 0; unsigned long reada_blocks, reada_mask, cur_block = 0; int err = 0; _ -- OGAWA Hirofumi
[PATCH] fat: Fix memory allocation failure handling of match_strdup()
In parse_options(), if match_strdup() failed, parse_options() leaves
opts->iocharset in unexpected state (i.e. still pointing the freed
string). And this can be the cause of double free.
To fix, this initialize opts->iocharset always when freeing.
Reported-by: syzbot+90b8e10515ae88228...@syzkaller.appspotmail.com
Cc:
Signed-off-by: OGAWA Hirofumi
---
fs/fat/inode.c | 20 +---
1 file changed, 13 insertions(+), 7 deletions(-)
diff -puN fs/fat/inode.c~fat-fix-kmalloc-failure fs/fat/inode.c
--- linux/fs/fat/inode.c~fat-fix-kmalloc-failure2018-07-12
12:20:30.388600735 +0900
+++ linux-hirofumi/fs/fat/inode.c 2018-07-12 15:09:48.764070539 +0900
@@ -703,13 +703,21 @@ static void fat_set_state(struct super_b
brelse(bh);
}
+static void fat_reset_iocharset(struct fat_mount_options *opts)
+{
+ if (opts->iocharset != fat_default_iocharset) {
+ /* Note: opts->iocharset can be NULL here */
+ kfree(opts->iocharset);
+ opts->iocharset = fat_default_iocharset;
+ }
+}
+
static void delayed_free(struct rcu_head *p)
{
struct msdos_sb_info *sbi = container_of(p, struct msdos_sb_info, rcu);
unload_nls(sbi->nls_disk);
unload_nls(sbi->nls_io);
- if (sbi->options.iocharset != fat_default_iocharset)
- kfree(sbi->options.iocharset);
+ fat_reset_iocharset(>options);
kfree(sbi);
}
@@ -1124,7 +1132,7 @@ static int parse_options(struct super_bl
opts->fs_fmask = opts->fs_dmask = current_umask();
opts->allow_utime = -1;
opts->codepage = fat_default_codepage;
- opts->iocharset = fat_default_iocharset;
+ fat_reset_iocharset(opts);
if (is_vfat) {
opts->shortname = VFAT_SFN_DISPLAY_WINNT|VFAT_SFN_CREATE_WIN95;
opts->rodir = 0;
@@ -1281,8 +1289,7 @@ static int parse_options(struct super_bl
/* vfat specific */
case Opt_charset:
- if (opts->iocharset != fat_default_iocharset)
- kfree(opts->iocharset);
+ fat_reset_iocharset(opts);
iocharset = match_strdup([0]);
if (!iocharset)
return -ENOMEM;
@@ -1873,8 +1880,7 @@ out_fail:
iput(fat_inode);
unload_nls(sbi->nls_io);
unload_nls(sbi->nls_disk);
- if (sbi->options.iocharset != fat_default_iocharset)
- kfree(sbi->options.iocharset);
+ fat_reset_iocharset(>options);
sb->s_fs_info = NULL;
kfree(sbi);
return error;
_
--
OGAWA Hirofumi
[PATCH] fat: Fix memory allocation failure handling of match_strdup()
In parse_options(), if match_strdup() failed, parse_options() leaves
opts->iocharset in unexpected state (i.e. still pointing the freed
string). And this can be the cause of double free.
To fix, this initialize opts->iocharset always when freeing.
Reported-by: syzbot+90b8e10515ae88228...@syzkaller.appspotmail.com
Cc:
Signed-off-by: OGAWA Hirofumi
---
fs/fat/inode.c | 20 +---
1 file changed, 13 insertions(+), 7 deletions(-)
diff -puN fs/fat/inode.c~fat-fix-kmalloc-failure fs/fat/inode.c
--- linux/fs/fat/inode.c~fat-fix-kmalloc-failure2018-07-12
12:20:30.388600735 +0900
+++ linux-hirofumi/fs/fat/inode.c 2018-07-12 15:09:48.764070539 +0900
@@ -703,13 +703,21 @@ static void fat_set_state(struct super_b
brelse(bh);
}
+static void fat_reset_iocharset(struct fat_mount_options *opts)
+{
+ if (opts->iocharset != fat_default_iocharset) {
+ /* Note: opts->iocharset can be NULL here */
+ kfree(opts->iocharset);
+ opts->iocharset = fat_default_iocharset;
+ }
+}
+
static void delayed_free(struct rcu_head *p)
{
struct msdos_sb_info *sbi = container_of(p, struct msdos_sb_info, rcu);
unload_nls(sbi->nls_disk);
unload_nls(sbi->nls_io);
- if (sbi->options.iocharset != fat_default_iocharset)
- kfree(sbi->options.iocharset);
+ fat_reset_iocharset(>options);
kfree(sbi);
}
@@ -1124,7 +1132,7 @@ static int parse_options(struct super_bl
opts->fs_fmask = opts->fs_dmask = current_umask();
opts->allow_utime = -1;
opts->codepage = fat_default_codepage;
- opts->iocharset = fat_default_iocharset;
+ fat_reset_iocharset(opts);
if (is_vfat) {
opts->shortname = VFAT_SFN_DISPLAY_WINNT|VFAT_SFN_CREATE_WIN95;
opts->rodir = 0;
@@ -1281,8 +1289,7 @@ static int parse_options(struct super_bl
/* vfat specific */
case Opt_charset:
- if (opts->iocharset != fat_default_iocharset)
- kfree(opts->iocharset);
+ fat_reset_iocharset(opts);
iocharset = match_strdup([0]);
if (!iocharset)
return -ENOMEM;
@@ -1873,8 +1880,7 @@ out_fail:
iput(fat_inode);
unload_nls(sbi->nls_io);
unload_nls(sbi->nls_disk);
- if (sbi->options.iocharset != fat_default_iocharset)
- kfree(sbi->options.iocharset);
+ fat_reset_iocharset(>options);
sb->s_fs_info = NULL;
kfree(sbi);
return error;
_
--
OGAWA Hirofumi
[PATCH] fat: Add FITRIM ioctl for FAT file system
From: Wentao Wang
Add FITRIM ioctl for FAT file system
[hirof...@mail.parknet.co.jp: bug fixes, coding style fixes, add signal check]
Signed-off-by: Wentao Wang
Signed-off-by: OGAWA Hirofumi
---
fs/fat/fat.h|1
fs/fat/fatent.c | 102 +++
fs/fat/file.c | 33 +
3 files changed, 136 insertions(+)
diff -puN fs/fat/fat.h~fat-fitrim fs/fat/fat.h
--- linux/fs/fat/fat.h~fat-fitrim 2018-06-04 13:55:42.328356395 +0900
+++ linux-hirofumi/fs/fat/fat.h 2018-06-04 13:55:42.331356390 +0900
@@ -357,6 +357,7 @@ extern int fat_alloc_clusters(struct ino
int nr_cluster);
extern int fat_free_clusters(struct inode *inode, int cluster);
extern int fat_count_free_clusters(struct super_block *sb);
+extern int fat_trim_fs(struct inode *inode, struct fstrim_range *range);
/* fat/file.c */
extern long fat_generic_ioctl(struct file *filp, unsigned int cmd,
diff -puN fs/fat/fatent.c~fat-fitrim fs/fat/fatent.c
--- linux/fs/fat/fatent.c~fat-fitrim2018-06-04 13:55:42.329356393 +0900
+++ linux-hirofumi/fs/fat/fatent.c 2018-06-04 14:00:18.550894746 +0900
@@ -4,6 +4,7 @@
*/
#include
+#include
#include "fat.h"
struct fatent_operations {
@@ -690,3 +691,104 @@ out:
unlock_fat(sbi);
return err;
}
+
+static int fat_trim_clusters(struct super_block *sb, u32 clus, u32 nr_clus)
+{
+ struct msdos_sb_info *sbi = MSDOS_SB(sb);
+ return sb_issue_discard(sb, fat_clus_to_blknr(sbi, clus),
+ nr_clus * sbi->sec_per_clus, GFP_NOFS, 0);
+}
+
+int fat_trim_fs(struct inode *inode, struct fstrim_range *range)
+{
+ struct super_block *sb = inode->i_sb;
+ struct msdos_sb_info *sbi = MSDOS_SB(sb);
+ const struct fatent_operations *ops = sbi->fatent_ops;
+ struct fat_entry fatent;
+ u64 ent_start, ent_end, minlen;
+ u32 free = 0, trimmed = 0;
+ unsigned long reada_blocks, reada_mask, cur_block = 0;
+ int err = 0;
+
+ /*
+* FAT data is organized as clusters, trim at the granulary of cluster.
+*
+* fstrim_range is in byte, convert vaules to cluster index.
+* Treat sectors before data region as all used, not to trim them.
+*/
+ ent_start = max_t(u64, range->start>>sbi->cluster_bits, FAT_START_ENT);
+ ent_end = ent_start + (range->len >> sbi->cluster_bits) - 1;
+ minlen = range->minlen >> sbi->cluster_bits;
+
+ if (ent_start >= sbi->max_cluster || range->len < sbi->cluster_size)
+ return -EINVAL;
+ if (ent_end >= sbi->max_cluster)
+ ent_end = sbi->max_cluster - 1;
+
+ reada_blocks = FAT_READA_SIZE >> sb->s_blocksize_bits;
+ reada_mask = reada_blocks - 1;
+
+ fatent_init();
+ lock_fat(sbi);
+ fatent_set_entry(, ent_start);
+ while (fatent.entry <= ent_end) {
+ /* readahead of fat blocks */
+ if ((cur_block & reada_mask) == 0) {
+ unsigned long rest = sbi->fat_length - cur_block;
+ fat_ent_reada(sb, , min(reada_blocks, rest));
+ }
+ cur_block++;
+
+ err = fat_ent_read_block(sb, );
+ if (err)
+ goto error;
+ do {
+ if (ops->ent_get() == FAT_ENT_FREE) {
+ free++;
+ } else if (free) {
+ if (free >= minlen) {
+ u32 clus = fatent.entry - free;
+
+ err = fat_trim_clusters(sb, clus, free);
+ if (err && err != -EOPNOTSUPP)
+ goto error;
+ if (!err)
+ trimmed += free;
+ err = 0;
+ }
+ free = 0;
+ }
+ } while (fat_ent_next(sbi, ) && fatent.entry <= ent_end);
+
+ if (fatal_signal_pending(current)) {
+ err = -ERESTARTSYS;
+ goto error;
+ }
+
+ if (need_resched()) {
+ fatent_brelse();
+ unlock_fat(sbi);
+ cond_resched();
+ lock_fat(sbi);
+ }
+ }
+ /* handle scenario when tail entries are all free */
+ if (free && free >= minlen) {
+ u32 clus = fatent.entry - free;
+
+ err = fat_trim_clusters(sb, clus, free);
+ if (err && err != -EOPNOTSUPP)
+ got
[PATCH] fat: Add FITRIM ioctl for FAT file system
From: Wentao Wang
Add FITRIM ioctl for FAT file system
[hirof...@mail.parknet.co.jp: bug fixes, coding style fixes, add signal check]
Signed-off-by: Wentao Wang
Signed-off-by: OGAWA Hirofumi
---
fs/fat/fat.h|1
fs/fat/fatent.c | 102 +++
fs/fat/file.c | 33 +
3 files changed, 136 insertions(+)
diff -puN fs/fat/fat.h~fat-fitrim fs/fat/fat.h
--- linux/fs/fat/fat.h~fat-fitrim 2018-06-04 13:55:42.328356395 +0900
+++ linux-hirofumi/fs/fat/fat.h 2018-06-04 13:55:42.331356390 +0900
@@ -357,6 +357,7 @@ extern int fat_alloc_clusters(struct ino
int nr_cluster);
extern int fat_free_clusters(struct inode *inode, int cluster);
extern int fat_count_free_clusters(struct super_block *sb);
+extern int fat_trim_fs(struct inode *inode, struct fstrim_range *range);
/* fat/file.c */
extern long fat_generic_ioctl(struct file *filp, unsigned int cmd,
diff -puN fs/fat/fatent.c~fat-fitrim fs/fat/fatent.c
--- linux/fs/fat/fatent.c~fat-fitrim2018-06-04 13:55:42.329356393 +0900
+++ linux-hirofumi/fs/fat/fatent.c 2018-06-04 14:00:18.550894746 +0900
@@ -4,6 +4,7 @@
*/
#include
+#include
#include "fat.h"
struct fatent_operations {
@@ -690,3 +691,104 @@ out:
unlock_fat(sbi);
return err;
}
+
+static int fat_trim_clusters(struct super_block *sb, u32 clus, u32 nr_clus)
+{
+ struct msdos_sb_info *sbi = MSDOS_SB(sb);
+ return sb_issue_discard(sb, fat_clus_to_blknr(sbi, clus),
+ nr_clus * sbi->sec_per_clus, GFP_NOFS, 0);
+}
+
+int fat_trim_fs(struct inode *inode, struct fstrim_range *range)
+{
+ struct super_block *sb = inode->i_sb;
+ struct msdos_sb_info *sbi = MSDOS_SB(sb);
+ const struct fatent_operations *ops = sbi->fatent_ops;
+ struct fat_entry fatent;
+ u64 ent_start, ent_end, minlen;
+ u32 free = 0, trimmed = 0;
+ unsigned long reada_blocks, reada_mask, cur_block = 0;
+ int err = 0;
+
+ /*
+* FAT data is organized as clusters, trim at the granulary of cluster.
+*
+* fstrim_range is in byte, convert vaules to cluster index.
+* Treat sectors before data region as all used, not to trim them.
+*/
+ ent_start = max_t(u64, range->start>>sbi->cluster_bits, FAT_START_ENT);
+ ent_end = ent_start + (range->len >> sbi->cluster_bits) - 1;
+ minlen = range->minlen >> sbi->cluster_bits;
+
+ if (ent_start >= sbi->max_cluster || range->len < sbi->cluster_size)
+ return -EINVAL;
+ if (ent_end >= sbi->max_cluster)
+ ent_end = sbi->max_cluster - 1;
+
+ reada_blocks = FAT_READA_SIZE >> sb->s_blocksize_bits;
+ reada_mask = reada_blocks - 1;
+
+ fatent_init();
+ lock_fat(sbi);
+ fatent_set_entry(, ent_start);
+ while (fatent.entry <= ent_end) {
+ /* readahead of fat blocks */
+ if ((cur_block & reada_mask) == 0) {
+ unsigned long rest = sbi->fat_length - cur_block;
+ fat_ent_reada(sb, , min(reada_blocks, rest));
+ }
+ cur_block++;
+
+ err = fat_ent_read_block(sb, );
+ if (err)
+ goto error;
+ do {
+ if (ops->ent_get() == FAT_ENT_FREE) {
+ free++;
+ } else if (free) {
+ if (free >= minlen) {
+ u32 clus = fatent.entry - free;
+
+ err = fat_trim_clusters(sb, clus, free);
+ if (err && err != -EOPNOTSUPP)
+ goto error;
+ if (!err)
+ trimmed += free;
+ err = 0;
+ }
+ free = 0;
+ }
+ } while (fat_ent_next(sbi, ) && fatent.entry <= ent_end);
+
+ if (fatal_signal_pending(current)) {
+ err = -ERESTARTSYS;
+ goto error;
+ }
+
+ if (need_resched()) {
+ fatent_brelse();
+ unlock_fat(sbi);
+ cond_resched();
+ lock_fat(sbi);
+ }
+ }
+ /* handle scenario when tail entries are all free */
+ if (free && free >= minlen) {
+ u32 clus = fatent.entry - free;
+
+ err = fat_trim_clusters(sb, clus, free);
+ if (err && err != -EOPNOTSUPP)
+ got
Re: PROBLEM: [kernel BUG at fs/fat/inode.c:162] when writing to a broken VFAT
Anatoly Trosinenko writes:
> Description:
>
> Writing to some file on a broken VFAT partition causes kernel bug
Thanks. This patch should fix this issue.
--
OGAWA Hirofumi
[PATCH] fat: Use fat_fs_error() instead of BUG_ON() in __fat_get_block()
If file size and FAT cluster chain is not matched (corrupted image),
we can hit BUG_ON(!phys) in __fat_get_block().
So, use fat_fs_error() instead.
Signed-off-by: OGAWA Hirofumi
---
fs/fat/inode.c |7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff -puN fs/fat/inode.c~vfat-dont-bugon fs/fat/inode.c
--- linux/fs/fat/inode.c~vfat-dont-bugon2018-06-02 20:15:04.441920069
+0900
+++ linux-hirofumi/fs/fat/inode.c 2018-06-02 20:15:04.442920067 +0900
@@ -158,8 +158,13 @@ static inline int __fat_get_block(struct
err = fat_bmap(inode, iblock, , _blocks, create, false);
if (err)
return err;
+ if (!phys) {
+ fat_fs_error(sb,
+"invalid FAT chain (i_pos %lld, last_block %ld)",
+MSDOS_I(inode)->i_pos, last_block);
+ return -EIO;
+ }
- BUG_ON(!phys);
BUG_ON(*max_blocks != mapped_blocks);
set_buffer_new(bh_result);
map_bh(bh_result, sb, phys);
_
Re: PROBLEM: [kernel BUG at fs/fat/inode.c:162] when writing to a broken VFAT
Anatoly Trosinenko writes:
> Description:
>
> Writing to some file on a broken VFAT partition causes kernel bug
Thanks. This patch should fix this issue.
--
OGAWA Hirofumi
[PATCH] fat: Use fat_fs_error() instead of BUG_ON() in __fat_get_block()
If file size and FAT cluster chain is not matched (corrupted image),
we can hit BUG_ON(!phys) in __fat_get_block().
So, use fat_fs_error() instead.
Signed-off-by: OGAWA Hirofumi
---
fs/fat/inode.c |7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff -puN fs/fat/inode.c~vfat-dont-bugon fs/fat/inode.c
--- linux/fs/fat/inode.c~vfat-dont-bugon2018-06-02 20:15:04.441920069
+0900
+++ linux-hirofumi/fs/fat/inode.c 2018-06-02 20:15:04.442920067 +0900
@@ -158,8 +158,13 @@ static inline int __fat_get_block(struct
err = fat_bmap(inode, iblock, , _blocks, create, false);
if (err)
return err;
+ if (!phys) {
+ fat_fs_error(sb,
+"invalid FAT chain (i_pos %lld, last_block %ld)",
+MSDOS_I(inode)->i_pos, last_block);
+ return -EIO;
+ }
- BUG_ON(!phys);
BUG_ON(*max_blocks != mapped_blocks);
set_buffer_new(bh_result);
map_bh(bh_result, sb, phys);
_
Re: [PATCH 3/3] fs: fat: add ioctl method in fat filesystem driver
chenchacha <chen.chencha...@foxmail.com> writes:
> On 01/29/2018 09:02 PM, OGAWA Hirofumi wrote:
>> ChenGuanqiao <chen.chencha...@foxmail.com> writes:
>>
>>> +static int fat_check_d_characters(char *label, unsigned long len)
>>> +{
>>> + int i;
>>> +
>>> + for (i = 0; i < len; ++i) {
>>> + switch (label[i]) {
>>> + case 'a' ... 'z':
>>> + label[i] = __toupper(label[i]);
>>> + case 'A' ... 'Z':
>>> + case '0' ... '9':
>>> + case '_':
>>> + case 0x20:
>>> + continue;
>>> + default:
>>> + return -EINVAL;
>>> + }
>> Same question with previous though, what windows do if label = "a b c"?
>> (this is including space other than end of name or extension.)
> In win7, the volume label will be capitalized, and leaving spaces.
> Or, you mean I need to fill the rest of the space with "0x20"?
I see. However, what win7 stored, BTW? It was "A B C ", or anything
other?
>>> +static int fat_ioctl_set_volume_label(struct file *file,
>>> + u8 __user *vol_label)
>>> +{
>>> + int err = 0;
>>> + u8 label[MSDOS_NAME];
>>> + struct timespec ts;
>>> + struct buffer_head *boot_bh;
>>> + struct buffer_head *vol_bh;
>>> + struct msdos_dir_entry *de;
>>> + struct fat_boot_sector *b;
>>> + struct inode *inode = file_inode(file);
>>> + struct super_block *sb = inode->i_sb;
>>> + struct msdos_sb_info *sbi = MSDOS_SB(sb);
>>> +
>> [...]
> I need remove "struct msdos_sb_info *sbi"?
If you didn't use sbi anymore, you should remove.
>>> + err = mnt_want_write_file(file);
>>> + if (err)
>>> + goto out;
>>> +
>>> + down_write(>s_umount);
>> Looks like inode_lock() for rootdir is gone. It is necessary to
>> traverse+modify.
> Is it wrong for me to use the inode_lock() in patch v7? I need to lock
> inode here, and turn off immediately after
inode_lock() is necessary to protect race with other dir operations.
I asked at v7, locking order to prevent the AB locking order bug.
I.e.
mnt_want_write_file => down_write => inode_lock()
vs
down_write => mnt_want_write_file => inode_lock()
Which is right one?
> mark_buffer_dirty(vol_bh)?
What is this asking?
--
OGAWA Hirofumi <hirof...@mail.parknet.co.jp>
Re: [PATCH 3/3] fs: fat: add ioctl method in fat filesystem driver
chenchacha writes:
> On 01/29/2018 09:02 PM, OGAWA Hirofumi wrote:
>> ChenGuanqiao writes:
>>
>>> +static int fat_check_d_characters(char *label, unsigned long len)
>>> +{
>>> + int i;
>>> +
>>> + for (i = 0; i < len; ++i) {
>>> + switch (label[i]) {
>>> + case 'a' ... 'z':
>>> + label[i] = __toupper(label[i]);
>>> + case 'A' ... 'Z':
>>> + case '0' ... '9':
>>> + case '_':
>>> + case 0x20:
>>> + continue;
>>> + default:
>>> + return -EINVAL;
>>> + }
>> Same question with previous though, what windows do if label = "a b c"?
>> (this is including space other than end of name or extension.)
> In win7, the volume label will be capitalized, and leaving spaces.
> Or, you mean I need to fill the rest of the space with "0x20"?
I see. However, what win7 stored, BTW? It was "A B C ", or anything
other?
>>> +static int fat_ioctl_set_volume_label(struct file *file,
>>> + u8 __user *vol_label)
>>> +{
>>> + int err = 0;
>>> + u8 label[MSDOS_NAME];
>>> + struct timespec ts;
>>> + struct buffer_head *boot_bh;
>>> + struct buffer_head *vol_bh;
>>> + struct msdos_dir_entry *de;
>>> + struct fat_boot_sector *b;
>>> + struct inode *inode = file_inode(file);
>>> + struct super_block *sb = inode->i_sb;
>>> + struct msdos_sb_info *sbi = MSDOS_SB(sb);
>>> +
>> [...]
> I need remove "struct msdos_sb_info *sbi"?
If you didn't use sbi anymore, you should remove.
>>> + err = mnt_want_write_file(file);
>>> + if (err)
>>> + goto out;
>>> +
>>> + down_write(>s_umount);
>> Looks like inode_lock() for rootdir is gone. It is necessary to
>> traverse+modify.
> Is it wrong for me to use the inode_lock() in patch v7? I need to lock
> inode here, and turn off immediately after
inode_lock() is necessary to protect race with other dir operations.
I asked at v7, locking order to prevent the AB locking order bug.
I.e.
mnt_want_write_file => down_write => inode_lock()
vs
down_write => mnt_want_write_file => inode_lock()
Which is right one?
> mark_buffer_dirty(vol_bh)?
What is this asking?
--
OGAWA Hirofumi
Re: [PATCH 3/3] fs: fat: add ioctl method in fat filesystem driver
ChenGuanqiao <chen.chencha...@foxmail.com> writes:
> +static int fat_check_d_characters(char *label, unsigned long len)
> +{
> + int i;
> +
> + for (i = 0; i < len; ++i) {
> + switch (label[i]) {
> + case 'a' ... 'z':
> + label[i] = __toupper(label[i]);
> + case 'A' ... 'Z':
> + case '0' ... '9':
> + case '_':
> + case 0x20:
> + continue;
> + default:
> + return -EINVAL;
> + }
Same question with previous though, what windows do if label = "a b c"?
(this is including space other than end of name or extension.)
> + }
> +
> + return 0;
> +}
> +static int fat_ioctl_set_volume_label(struct file *file,
> + u8 __user *vol_label)
> +{
> + int err = 0;
> + u8 label[MSDOS_NAME];
> + struct timespec ts;
> + struct buffer_head *boot_bh;
> + struct buffer_head *vol_bh;
> + struct msdos_dir_entry *de;
> + struct fat_boot_sector *b;
> + struct inode *inode = file_inode(file);
> + struct super_block *sb = inode->i_sb;
> + struct msdos_sb_info *sbi = MSDOS_SB(sb);
> +
[...]
> + err = mnt_want_write_file(file);
> + if (err)
> + goto out;
> +
> + down_write(>s_umount);
Looks like inode_lock() for rootdir is gone. It is necessary to
traverse+modify.
> + /* Updates root directory's vol_label */
> + err = fat_get_volume_label_entry(inode, _bh, );
> + ts = current_time(inode);
> + if (err) {
> + /* Create volume label entry */
> + err = fat_add_volume_label_entry(inode, label, );
> + if (err)
> + goto out_vol_brelse;
> + } else {
> + /* Write to root directory */
> + memcpy(de->name, label, sizeof(de->name));
> + inode->i_ctime = inode->i_mtime = inode->i_atime = ts;
> +
> + mark_buffer_dirty(vol_bh);
> + }
> +
> + /* Update sector's vol_label */
> + boot_bh = sb_bread(sb, 0);
> + if (boot_bh == NULL) {
> + fat_msg(sb, KERN_ERR,
> + "unable to read boot sector to write volume label");
> + err = -EIO;
> + goto out_boot_brelse;
> + }
> +
> + b = (struct fat_boot_sector *)boot_bh->b_data;
> + if (sbi->fat_bits == 32)
> + memcpy(b->fat32.vol_label, label, sizeof(label));
> + else
> + memcpy(b->fat16.vol_label, label, sizeof(label));
> +
> + mark_buffer_dirty(boot_bh);
> +
> + /* Synchronize the data together */
> + err = sync_dirty_buffer(boot_bh);
> + if (err)
> + goto out_boot_brelse;
--
OGAWA Hirofumi <hirof...@mail.parknet.co.jp>
Re: [PATCH 3/3] fs: fat: add ioctl method in fat filesystem driver
ChenGuanqiao writes:
> +static int fat_check_d_characters(char *label, unsigned long len)
> +{
> + int i;
> +
> + for (i = 0; i < len; ++i) {
> + switch (label[i]) {
> + case 'a' ... 'z':
> + label[i] = __toupper(label[i]);
> + case 'A' ... 'Z':
> + case '0' ... '9':
> + case '_':
> + case 0x20:
> + continue;
> + default:
> + return -EINVAL;
> + }
Same question with previous though, what windows do if label = "a b c"?
(this is including space other than end of name or extension.)
> + }
> +
> + return 0;
> +}
> +static int fat_ioctl_set_volume_label(struct file *file,
> + u8 __user *vol_label)
> +{
> + int err = 0;
> + u8 label[MSDOS_NAME];
> + struct timespec ts;
> + struct buffer_head *boot_bh;
> + struct buffer_head *vol_bh;
> + struct msdos_dir_entry *de;
> + struct fat_boot_sector *b;
> + struct inode *inode = file_inode(file);
> + struct super_block *sb = inode->i_sb;
> + struct msdos_sb_info *sbi = MSDOS_SB(sb);
> +
[...]
> + err = mnt_want_write_file(file);
> + if (err)
> + goto out;
> +
> + down_write(>s_umount);
Looks like inode_lock() for rootdir is gone. It is necessary to
traverse+modify.
> + /* Updates root directory's vol_label */
> + err = fat_get_volume_label_entry(inode, _bh, );
> + ts = current_time(inode);
> + if (err) {
> + /* Create volume label entry */
> + err = fat_add_volume_label_entry(inode, label, );
> + if (err)
> + goto out_vol_brelse;
> + } else {
> + /* Write to root directory */
> + memcpy(de->name, label, sizeof(de->name));
> + inode->i_ctime = inode->i_mtime = inode->i_atime = ts;
> +
> + mark_buffer_dirty(vol_bh);
> + }
> +
> + /* Update sector's vol_label */
> + boot_bh = sb_bread(sb, 0);
> + if (boot_bh == NULL) {
> + fat_msg(sb, KERN_ERR,
> + "unable to read boot sector to write volume label");
> + err = -EIO;
> + goto out_boot_brelse;
> + }
> +
> + b = (struct fat_boot_sector *)boot_bh->b_data;
> + if (sbi->fat_bits == 32)
> + memcpy(b->fat32.vol_label, label, sizeof(label));
> + else
> + memcpy(b->fat16.vol_label, label, sizeof(label));
> +
> + mark_buffer_dirty(boot_bh);
> +
> + /* Synchronize the data together */
> + err = sync_dirty_buffer(boot_bh);
> + if (err)
> + goto out_boot_brelse;
--
OGAWA Hirofumi
Re: [PATCH v7 1/3] fs: fat: Add fat filesystem partition volume label in local structure
ChenGuanqiao <chen.chencha...@foxmail.com> writes:
> Signed-off-by: ChenGuanqiao <chen.chencha...@foxmail.com>
> ---
> fs/fat/fat.h | 6 ++
> fs/fat/inode.c| 15 ---
> include/uapi/linux/msdos_fs.h | 2 ++
> 3 files changed, 20 insertions(+), 3 deletions(-)
>
> diff --git a/fs/fat/fat.h b/fs/fat/fat.h
> index 8fc1093da47d..96f631282c71 100644
> --- a/fs/fat/fat.h
> +++ b/fs/fat/fat.h
> @@ -86,6 +86,7 @@ struct msdos_sb_info {
> int dir_per_block;/* dir entries per block */
> int dir_per_block_bits; /* log2(dir_per_block) */
> unsigned int vol_id;/*volume ID*/
> + char vol_label[11]; /*volume label*/
In latter patches doesn't use this cache. Why do we cache, or why read
ioctl doesn't use this cache?
--
OGAWA Hirofumi <hirof...@mail.parknet.co.jp>
Re: [PATCH v7 1/3] fs: fat: Add fat filesystem partition volume label in local structure
ChenGuanqiao writes:
> Signed-off-by: ChenGuanqiao
> ---
> fs/fat/fat.h | 6 ++
> fs/fat/inode.c| 15 ---
> include/uapi/linux/msdos_fs.h | 2 ++
> 3 files changed, 20 insertions(+), 3 deletions(-)
>
> diff --git a/fs/fat/fat.h b/fs/fat/fat.h
> index 8fc1093da47d..96f631282c71 100644
> --- a/fs/fat/fat.h
> +++ b/fs/fat/fat.h
> @@ -86,6 +86,7 @@ struct msdos_sb_info {
> int dir_per_block;/* dir entries per block */
> int dir_per_block_bits; /* log2(dir_per_block) */
> unsigned int vol_id;/*volume ID*/
> + char vol_label[11]; /*volume label*/
In latter patches doesn't use this cache. Why do we cache, or why read
ioctl doesn't use this cache?
--
OGAWA Hirofumi
Re: [PATCH v7 3/3] fs: fat: add ioctl method in fat filesystem driver
ChenGuanqiao <chen.chencha...@foxmail.com> writes:
> +static int fat_check_d_characters(char *label, unsigned long len)
> +{
> + int i;
> +
> + for (i = 0; i < len; ++i) {
> + switch (label[i]) {
> + case '0' ... '9':
> + case 'A' ... 'Z':
> + case '_':
> + case 0x20:
> + continue;
I didn't check though, what windows do if label = "a b c"? (I.e. invalid
name as dirent name)
> +static int fat_ioctl_get_volume_label(struct inode *inode,
> + u8 __user *vol_label)
> +{
> + int err = 0;
> + struct buffer_head *bh;
> + struct msdos_dir_entry *de;
> + struct super_block *sb = inode->i_sb;
> + char buffer[MSDOS_NAME] = {0};
Why initialize buffer?
> + inode = d_inode(sb->s_root);
> +
> + err = fat_get_volume_label_entry(inode, , );
> + if (err)
> + goto out;
The dir traverse has to be had lock.
> + inode_lock_shared(inode);
> + memcpy(buffer, de->name, MSDOS_NAME);
> + brelse(bh);
> + inode_unlock_shared(inode);
> +
> + if (copy_to_user(vol_label, buffer, MSDOS_NAME))
> + err = -EFAULT;
No issue to copy to user memory under locking.
> +static int fat_ioctl_set_volume_label(struct file *file,
> + u8 __user *vol_label)
> +{
[...]
> + err = fat_check_d_characters(label, sizeof(label));
> + if (err)
> + goto out;
> +
> + err = mnt_want_write_file(file);
> + if (err)
> + goto out;
> +
> + down_write(>s_umount);
> + inode_lock(inode);
BTW, lock order is tested with LOCKDEP?
> + /* Synchronize the data together */
> + err = sync_dirty_buffer(boot_bh);
> + if (err)
> + goto out_boot_brelse;
> + err = sync_dirty_buffer(vol_bh);
> + if (err)
> + goto out_boot_brelse;
Probably, we don't need to sync vol_bh. And in the case of adding new
entry, now already doesn't sync.
--
OGAWA Hirofumi <hirof...@mail.parknet.co.jp>
Re: [PATCH v7 3/3] fs: fat: add ioctl method in fat filesystem driver
ChenGuanqiao writes:
> +static int fat_check_d_characters(char *label, unsigned long len)
> +{
> + int i;
> +
> + for (i = 0; i < len; ++i) {
> + switch (label[i]) {
> + case '0' ... '9':
> + case 'A' ... 'Z':
> + case '_':
> + case 0x20:
> + continue;
I didn't check though, what windows do if label = "a b c"? (I.e. invalid
name as dirent name)
> +static int fat_ioctl_get_volume_label(struct inode *inode,
> + u8 __user *vol_label)
> +{
> + int err = 0;
> + struct buffer_head *bh;
> + struct msdos_dir_entry *de;
> + struct super_block *sb = inode->i_sb;
> + char buffer[MSDOS_NAME] = {0};
Why initialize buffer?
> + inode = d_inode(sb->s_root);
> +
> + err = fat_get_volume_label_entry(inode, , );
> + if (err)
> + goto out;
The dir traverse has to be had lock.
> + inode_lock_shared(inode);
> + memcpy(buffer, de->name, MSDOS_NAME);
> + brelse(bh);
> + inode_unlock_shared(inode);
> +
> + if (copy_to_user(vol_label, buffer, MSDOS_NAME))
> + err = -EFAULT;
No issue to copy to user memory under locking.
> +static int fat_ioctl_set_volume_label(struct file *file,
> + u8 __user *vol_label)
> +{
[...]
> + err = fat_check_d_characters(label, sizeof(label));
> + if (err)
> + goto out;
> +
> + err = mnt_want_write_file(file);
> + if (err)
> + goto out;
> +
> + down_write(>s_umount);
> + inode_lock(inode);
BTW, lock order is tested with LOCKDEP?
> + /* Synchronize the data together */
> + err = sync_dirty_buffer(boot_bh);
> + if (err)
> + goto out_boot_brelse;
> + err = sync_dirty_buffer(vol_bh);
> + if (err)
> + goto out_boot_brelse;
Probably, we don't need to sync vol_bh. And in the case of adding new
entry, now already doesn't sync.
--
OGAWA Hirofumi
Re: [PATCH v7 2/3] fs: fat: Add volume label entry method function
ChenGuanqiao <chen.chencha...@foxmail.com> writes:
> Signed-off-by: ChenGuanqiao <chen.chencha...@foxmail.com>
> ---
> fs/fat/dir.c | 53 +
> 1 file changed, 53 insertions(+)
>
> diff --git a/fs/fat/dir.c b/fs/fat/dir.c
> index 8e100c3bf72c..53f36c03760e 100644
> --- a/fs/fat/dir.c
> +++ b/fs/fat/dir.c
> @@ -881,6 +881,59 @@ static int fat_get_short_entry(struct inode *dir, loff_t
> *pos,
> return -ENOENT;
> }
>
> +int fat_get_volume_label_entry(struct inode *dir, struct buffer_head **bh,
> +struct msdos_dir_entry **de)
> +{
> + loff_t pos = 0;
> +
> + if (dir->i_ino != MSDOS_ROOT_INO)
> + return -EFAULT;
Now, dir is always root, so this check doesn't matter.
> + *bh = NULL;
> + *de = NULL;
> + while (fat_get_entry(dir, , bh, de) >= 0) {
> + if (((*de)->attr & ATTR_VOLUME) && ((*de)->attr != ATTR_EXT) &&
> + !IS_FREE((*de)->name))
> + return 0;
> + }
> + return -ENOENT;
> +}
> +EXPORT_SYMBOL_GPL(fat_get_volume_label_entry);
> +
> +int fat_add_volume_label_entry(struct inode *dir, const unsigned char *name,
> +struct timespec *ts)
> +{
> + struct msdos_sb_info *sbi = MSDOS_SB(dir->i_sb);
> + struct msdos_dir_entry de;
> + struct fat_slot_info sinfo;
> + __le16 time, date;
> + int err;
> +
> + if (dir->i_ino != MSDOS_ROOT_INO)
> + return -EFAULT;
Here is too. dir is always root.
> + memcpy(de.name, name, MSDOS_NAME);
> + de.attr = ATTR_VOLUME;
> + de.lcase = 0;
> + fat_time_unix2fat(sbi, ts, , , NULL);
> + de.cdate = de.adate = 0;
> + de.ctime = 0;
> + de.ctime_cs = 0;
> + de.time = time;
> + de.date = date;
> + fat_set_start(, 0);
> + de.size = 0;
> +
> + err = fat_add_entries(dir, , 1, );
> + if (err)
> + return err;
> +
> + brelse(sinfo.bh);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(fat_add_volume_label_entry);
--
OGAWA Hirofumi <hirof...@mail.parknet.co.jp>
Re: [PATCH v7 2/3] fs: fat: Add volume label entry method function
ChenGuanqiao writes:
> Signed-off-by: ChenGuanqiao
> ---
> fs/fat/dir.c | 53 +
> 1 file changed, 53 insertions(+)
>
> diff --git a/fs/fat/dir.c b/fs/fat/dir.c
> index 8e100c3bf72c..53f36c03760e 100644
> --- a/fs/fat/dir.c
> +++ b/fs/fat/dir.c
> @@ -881,6 +881,59 @@ static int fat_get_short_entry(struct inode *dir, loff_t
> *pos,
> return -ENOENT;
> }
>
> +int fat_get_volume_label_entry(struct inode *dir, struct buffer_head **bh,
> +struct msdos_dir_entry **de)
> +{
> + loff_t pos = 0;
> +
> + if (dir->i_ino != MSDOS_ROOT_INO)
> + return -EFAULT;
Now, dir is always root, so this check doesn't matter.
> + *bh = NULL;
> + *de = NULL;
> + while (fat_get_entry(dir, , bh, de) >= 0) {
> + if (((*de)->attr & ATTR_VOLUME) && ((*de)->attr != ATTR_EXT) &&
> + !IS_FREE((*de)->name))
> + return 0;
> + }
> + return -ENOENT;
> +}
> +EXPORT_SYMBOL_GPL(fat_get_volume_label_entry);
> +
> +int fat_add_volume_label_entry(struct inode *dir, const unsigned char *name,
> +struct timespec *ts)
> +{
> + struct msdos_sb_info *sbi = MSDOS_SB(dir->i_sb);
> + struct msdos_dir_entry de;
> + struct fat_slot_info sinfo;
> + __le16 time, date;
> + int err;
> +
> + if (dir->i_ino != MSDOS_ROOT_INO)
> + return -EFAULT;
Here is too. dir is always root.
> + memcpy(de.name, name, MSDOS_NAME);
> + de.attr = ATTR_VOLUME;
> + de.lcase = 0;
> + fat_time_unix2fat(sbi, ts, , , NULL);
> + de.cdate = de.adate = 0;
> + de.ctime = 0;
> + de.ctime_cs = 0;
> + de.time = time;
> + de.date = date;
> + fat_set_start(, 0);
> + de.size = 0;
> +
> + err = fat_add_entries(dir, , 1, );
> + if (err)
> + return err;
> +
> + brelse(sinfo.bh);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(fat_add_volume_label_entry);
--
OGAWA Hirofumi
Re: [PATCH v6 3/3] fs: fat: add ioctl method in fat filesystem driver
ChenGuanqiao <chen.chencha...@foxmail.com> writes:
> +static int fat_check_d_characters(char *label, unsigned long len)
> +{
> + int i;
> +
> + for (i=0; i<len; ++i) {
coding style. "i=0" to "i = 0", etc.
> + switch (label[i]) {
> + case '0' ... '9':
> + case 'A' ... 'Z':
> + case '_':
> + continue;
> + case 0x20:
> + return 0;
Hm, stop check at ' '? What if "aaa b.%%%"?
> +static int fat_ioctl_get_volume_label(struct inode *inode,
> + u8 __user *vol_label)
> +{
> + int err = 0;
> + struct buffer_head *bh;
> + struct msdos_dir_entry *de;
Hm, user has to care to open rootdir for this ioctl? Isn't it better to
force use the root inode?
> + inode_lock_shared(inode);
> + err = fat_scan_volume_label(inode, , );
> + if (err)
> + goto out;
> +
> + if (copy_to_user(vol_label, de->name, MSDOS_NAME))
> + err = -EFAULT;
Better to copy to user buffer outside locking.
> +static int fat_ioctl_set_volume_label(struct file *file,
> + u8 __user *vol_label)
> +{
> + int err = 0;
> + u8 label[MSDOS_NAME];
> + struct buffer_head *boot_bh;
> + struct buffer_head *vol_bh;
> + struct msdos_dir_entry *de;
> + struct fat_boot_sector *b;
> + struct inode *inode = file_inode(file);
> + struct super_block *sb = inode->i_sb;
> + struct msdos_sb_info *sbi = MSDOS_SB(sb);
> +
> + if (inode->i_ino != MSDOS_ROOT_INO) {
Same with above, better to force use the root inode.
> + down_write(>s_umount);
> + inode_lock(inode);
> +
> + /* Updates root directory's vol_label */
> + err = fat_scan_volume_label(inode, _bh, );
> + if (err) {
> + /* Create volume label entry */
> + struct timespec ts;
> +
> + mutex_lock(>s_lock);
No need sbi->s_lock?
> + ts = current_time(inode);
> + err = fat_add_volume_label_entry(inode, label, );
> + mutex_unlock(>s_lock);
> +
> + if (err)
> + goto out_vol_brelse;
> + } else {
> + /* Write to root directory */
> + lock_buffer(vol_bh);
No need lock_buffer()?
> + memcpy(de->name, label, sizeof(de->name));
Probably, update timestamp?
> + mark_buffer_dirty(vol_bh);
> + unlock_buffer(vol_bh);
> + }
> +
> + /* Update sector's vol_label */
> + boot_bh = sb_bread(sb, 0);
> + if (boot_bh == NULL) {
> + fat_msg(sb, KERN_ERR,
> + "unable to read boot sector to write volume label");
> + err = -EIO;
> + goto out_boot_brelse;
> + }
> +
> + b = (struct fat_boot_sector *)boot_bh->b_data;
> + lock_buffer(boot_bh);
No need lock_buffer()?
> + if (sbi->fat_bits == 32)
> + memcpy(b->fat32.vol_label, label, sizeof(label));
> + else
> + memcpy(b->fat16.vol_label, label, sizeof(label));
> +
> + mark_buffer_dirty(boot_bh);
> + unlock_buffer(boot_bh);
--
OGAWA Hirofumi <hirof...@mail.parknet.co.jp>
Re: [PATCH v6 3/3] fs: fat: add ioctl method in fat filesystem driver
ChenGuanqiao writes:
> +static int fat_check_d_characters(char *label, unsigned long len)
> +{
> + int i;
> +
> + for (i=0; i + switch (label[i]) {
> + case '0' ... '9':
> + case 'A' ... 'Z':
> + case '_':
> + continue;
> + case 0x20:
> + return 0;
Hm, stop check at ' '? What if "aaa b.%%%"?
> +static int fat_ioctl_get_volume_label(struct inode *inode,
> + u8 __user *vol_label)
> +{
> + int err = 0;
> + struct buffer_head *bh;
> + struct msdos_dir_entry *de;
Hm, user has to care to open rootdir for this ioctl? Isn't it better to
force use the root inode?
> + inode_lock_shared(inode);
> + err = fat_scan_volume_label(inode, , );
> + if (err)
> + goto out;
> +
> + if (copy_to_user(vol_label, de->name, MSDOS_NAME))
> + err = -EFAULT;
Better to copy to user buffer outside locking.
> +static int fat_ioctl_set_volume_label(struct file *file,
> + u8 __user *vol_label)
> +{
> + int err = 0;
> + u8 label[MSDOS_NAME];
> + struct buffer_head *boot_bh;
> + struct buffer_head *vol_bh;
> + struct msdos_dir_entry *de;
> + struct fat_boot_sector *b;
> + struct inode *inode = file_inode(file);
> + struct super_block *sb = inode->i_sb;
> + struct msdos_sb_info *sbi = MSDOS_SB(sb);
> +
> + if (inode->i_ino != MSDOS_ROOT_INO) {
Same with above, better to force use the root inode.
> + down_write(>s_umount);
> + inode_lock(inode);
> +
> + /* Updates root directory's vol_label */
> + err = fat_scan_volume_label(inode, _bh, );
> + if (err) {
> + /* Create volume label entry */
> + struct timespec ts;
> +
> + mutex_lock(>s_lock);
No need sbi->s_lock?
> + ts = current_time(inode);
> + err = fat_add_volume_label_entry(inode, label, );
> + mutex_unlock(>s_lock);
> +
> + if (err)
> + goto out_vol_brelse;
> + } else {
> + /* Write to root directory */
> + lock_buffer(vol_bh);
No need lock_buffer()?
> + memcpy(de->name, label, sizeof(de->name));
Probably, update timestamp?
> + mark_buffer_dirty(vol_bh);
> + unlock_buffer(vol_bh);
> + }
> +
> + /* Update sector's vol_label */
> + boot_bh = sb_bread(sb, 0);
> + if (boot_bh == NULL) {
> + fat_msg(sb, KERN_ERR,
> + "unable to read boot sector to write volume label");
> + err = -EIO;
> + goto out_boot_brelse;
> + }
> +
> + b = (struct fat_boot_sector *)boot_bh->b_data;
> + lock_buffer(boot_bh);
No need lock_buffer()?
> + if (sbi->fat_bits == 32)
> + memcpy(b->fat32.vol_label, label, sizeof(label));
> + else
> + memcpy(b->fat16.vol_label, label, sizeof(label));
> +
> + mark_buffer_dirty(boot_bh);
> + unlock_buffer(boot_bh);
--
OGAWA Hirofumi
Re: [PATCH v6 2/3] fs: fat: Add volume label entry method function
ChenGuanqiao <chen.chencha...@foxmail.com> writes:
> +static int fat_get_volume_label_entry(struct inode *dir, loff_t *pos,
> +struct buffer_head **bh,
> +struct msdos_dir_entry **de)
> +{
> + while (fat_get_entry(dir, pos, bh, de) >= 0) {
> + if (((*de)->attr & ATTR_VOLUME) && (*de)->attr != ATTR_EXT)
It should check IS_FREE().
> + return 0;
> + }
> + return -ENOENT;
> +}
> +
> +int fat_scan_volume_label(struct inode *dir, struct buffer_head **bh,
> + struct msdos_dir_entry **de)
> +{
> + loff_t offset = 0;
> +
> + if (dir->i_ino != MSDOS_ROOT_INO)
> + return -EINVAL;
> +
> + *bh = NULL;
> + *de = NULL;
> + while (fat_get_volume_label_entry(dir, , bh, de) >= 0)
> + return 0;
> +
> + return -ENOENT;
> +}
> +EXPORT_SYMBOL_GPL(fat_scan_volume_label);
No need this wrapper? Looks like better to merge into
fat_get_volume_label_entry().
> +int fat_add_volume_label_entry(struct inode *dir, const unsigned char *name,
> +struct timespec *ts)
> +{
> + struct msdos_sb_info *sbi = MSDOS_SB(dir->i_sb);
> + struct msdos_dir_entry de;
> + struct fat_slot_info sinfo;
> + __le16 time, date;
> + int err;
> +
> + if (dir->i_ino != MSDOS_ROOT_INO)
> + return -EINVAL;
> +
> + memcpy(de.name, name, MSDOS_NAME);
> + de.attr = ATTR_VOLUME;
> + de.lcase = 0;
> + fat_time_unix2fat(sbi, ts, , , NULL);
> + de.cdate = de.adate = 0;
> + de.ctime = 0;
> + de.ctime_cs = 0;
> + de.time = time;
> + de.date = date;
> + fat_set_start(, 0);
> + de.size = 0;
> +
> + err = fat_add_entries(dir, , 1, );
> + if (err)
> + return err;
Missing call of brelse(sinfo.bh)
> + dir->i_ctime = dir->i_mtime = *ts;
Probably, we should not update this for labal.
> + return fat_sync_inode(dir);
No inode for root.
--
OGAWA Hirofumi <hirof...@mail.parknet.co.jp>
Re: [PATCH v6 2/3] fs: fat: Add volume label entry method function
ChenGuanqiao writes:
> +static int fat_get_volume_label_entry(struct inode *dir, loff_t *pos,
> +struct buffer_head **bh,
> +struct msdos_dir_entry **de)
> +{
> + while (fat_get_entry(dir, pos, bh, de) >= 0) {
> + if (((*de)->attr & ATTR_VOLUME) && (*de)->attr != ATTR_EXT)
It should check IS_FREE().
> + return 0;
> + }
> + return -ENOENT;
> +}
> +
> +int fat_scan_volume_label(struct inode *dir, struct buffer_head **bh,
> + struct msdos_dir_entry **de)
> +{
> + loff_t offset = 0;
> +
> + if (dir->i_ino != MSDOS_ROOT_INO)
> + return -EINVAL;
> +
> + *bh = NULL;
> + *de = NULL;
> + while (fat_get_volume_label_entry(dir, , bh, de) >= 0)
> + return 0;
> +
> + return -ENOENT;
> +}
> +EXPORT_SYMBOL_GPL(fat_scan_volume_label);
No need this wrapper? Looks like better to merge into
fat_get_volume_label_entry().
> +int fat_add_volume_label_entry(struct inode *dir, const unsigned char *name,
> +struct timespec *ts)
> +{
> + struct msdos_sb_info *sbi = MSDOS_SB(dir->i_sb);
> + struct msdos_dir_entry de;
> + struct fat_slot_info sinfo;
> + __le16 time, date;
> + int err;
> +
> + if (dir->i_ino != MSDOS_ROOT_INO)
> + return -EINVAL;
> +
> + memcpy(de.name, name, MSDOS_NAME);
> + de.attr = ATTR_VOLUME;
> + de.lcase = 0;
> + fat_time_unix2fat(sbi, ts, , , NULL);
> + de.cdate = de.adate = 0;
> + de.ctime = 0;
> + de.ctime_cs = 0;
> + de.time = time;
> + de.date = date;
> + fat_set_start(, 0);
> + de.size = 0;
> +
> + err = fat_add_entries(dir, , 1, );
> + if (err)
> + return err;
Missing call of brelse(sinfo.bh)
> + dir->i_ctime = dir->i_mtime = *ts;
Probably, we should not update this for labal.
> + return fat_sync_inode(dir);
No inode for root.
--
OGAWA Hirofumi
Re: [PATCH v5 2/2] fs: fat: add ioctl method in fat filesystem driver
ChenGuanqiao <chen.chencha...@foxmail.com> writes:
> +/* the characters in this field shall be d-characters, and unused byte shall
> be set to 0x20. */
> +static int fat_format_d_characters(char *label, unsigned long len)
> +{
> + int i;
> +
> + for (i=0; i<len; ++i) {
> + switch (label[i]) {
> + case '0' ... '9':
> + case 'A' ... 'Z':
> + case '_':
> + continue;
> + default:
> + break;
> + }
> + break;
> + }
> +
> + for (; i<len; ++i)
> + label[i] = 0x20;
> +
> + return len;
> +}
It should not accept and overwrite invalid char, return -EINVAL.
> +static int fat_ioctl_get_volume_label(struct inode *inode,
> + u8 __user *vol_label)
> +{
> + int err = 0;
> + struct fat_slot_info sinfo;
> +
> + err = fat_scan_volume_label(inode, );
> + if (err)
> + goto out;
It needs to take inode read lock.
> + if (copy_to_user(vol_label, sinfo.de->name, MSDOS_NAME))
> + err = -EFAULT;
> +
> + brelse(sinfo.bh);
> +out:
> + return err;
> +}
> +
> +static int fat_ioctl_set_volume_label(struct file *file,
> + u8 __user *vol_label)
> +{
[...]
> + b = (struct fat_boot_sector *)bh->b_data;
> +
> + lock_buffer(bh);
Probably, sb->s_umount to exclusive with remount update.
> + if (sbi->fat_bits == 32)
> + memcpy(b->fat32.vol_label, label, sizeof(label));
> + else
> + memcpy(b->fat16.vol_label, label, sizeof(label));
> +
> + mark_buffer_dirty(bh);
> + unlock_buffer(bh);
> + err = sync_dirty_buffer(bh);
> + brelse(bh);
> + if (err)
> + goto out_drop_file;
It should not update before preparing is done and checking error.
> + /* updates root directory's vol_label */
> + err = fat_scan_volume_label(inode, );
> + if (err)
> + goto out_drop_file;
It should add entry if no volume label.
> + lock_buffer(bh);
inode write lock.
> + memcpy(sinfo.de->name, label, sizeof(sinfo.de->name));
> + mark_buffer_dirty(bh);
> + unlock_buffer(bh);
Thanks.
--
OGAWA Hirofumi <hirof...@mail.parknet.co.jp>
Re: [PATCH v5 2/2] fs: fat: add ioctl method in fat filesystem driver
ChenGuanqiao writes:
> +/* the characters in this field shall be d-characters, and unused byte shall
> be set to 0x20. */
> +static int fat_format_d_characters(char *label, unsigned long len)
> +{
> + int i;
> +
> + for (i=0; i + switch (label[i]) {
> + case '0' ... '9':
> + case 'A' ... 'Z':
> + case '_':
> + continue;
> + default:
> + break;
> + }
> + break;
> + }
> +
> + for (; i + label[i] = 0x20;
> +
> + return len;
> +}
It should not accept and overwrite invalid char, return -EINVAL.
> +static int fat_ioctl_get_volume_label(struct inode *inode,
> + u8 __user *vol_label)
> +{
> + int err = 0;
> + struct fat_slot_info sinfo;
> +
> + err = fat_scan_volume_label(inode, );
> + if (err)
> + goto out;
It needs to take inode read lock.
> + if (copy_to_user(vol_label, sinfo.de->name, MSDOS_NAME))
> + err = -EFAULT;
> +
> + brelse(sinfo.bh);
> +out:
> + return err;
> +}
> +
> +static int fat_ioctl_set_volume_label(struct file *file,
> + u8 __user *vol_label)
> +{
[...]
> + b = (struct fat_boot_sector *)bh->b_data;
> +
> + lock_buffer(bh);
Probably, sb->s_umount to exclusive with remount update.
> + if (sbi->fat_bits == 32)
> + memcpy(b->fat32.vol_label, label, sizeof(label));
> + else
> + memcpy(b->fat16.vol_label, label, sizeof(label));
> +
> + mark_buffer_dirty(bh);
> + unlock_buffer(bh);
> + err = sync_dirty_buffer(bh);
> + brelse(bh);
> + if (err)
> + goto out_drop_file;
It should not update before preparing is done and checking error.
> + /* updates root directory's vol_label */
> + err = fat_scan_volume_label(inode, );
> + if (err)
> + goto out_drop_file;
It should add entry if no volume label.
> + lock_buffer(bh);
inode write lock.
> + memcpy(sinfo.de->name, label, sizeof(sinfo.de->name));
> + mark_buffer_dirty(bh);
> + unlock_buffer(bh);
Thanks.
--
OGAWA Hirofumi
Re: [PATCH v5 1/2] fs: fat: Add fat filesystem partition volume label in local structure
ChenGuanqiao <chen.chencha...@foxmail.com> writes:
> +static int fat_get_volume_label_entry(struct inode *dir, loff_t *pos,
> +struct buffer_head **bh,
> +struct msdos_dir_entry **de)
> +{
> + while (fat_get_entry(dir, pos, bh, de) >= 0)
> + if (((*de)->attr & ATTR_VOLUME) && (*de)->attr != ATTR_EXT)
> + return 0;
> + return -ENOENT;
> +}
> +
> +int fat_scan_volume_label(struct inode *dir, struct fat_slot_info *sinfo)
> +{
> + struct super_block *sb = dir->i_sb;
> +
> + sinfo->slot_off = 0;
> + sinfo->bh = NULL;
> + while (fat_get_volume_label_entry(dir, >slot_off,
> + >bh, >de) >= 0) {
> + sinfo->slot_off -= sizeof(*sinfo->de);
> + sinfo->nr_slots = 1;
> + sinfo->i_pos = fat_make_i_pos(sb, sinfo->bh, sinfo->de);
> +
> + return 0;
> + }
> +
> + return -ENOENT;
> +}
> +EXPORT_SYMBOL_GPL(fat_scan_volume_label);
Why do you need sinfo?
--
OGAWA Hirofumi <hirof...@mail.parknet.co.jp>
