[U-Boot] [PATCH] fs: fat: fix reading non-cluster-aligned root directory

[U-Boot] [PATCH] fs: fat: fix reading non-cluster-aligned root directory

[Anssi Hannula]

FAT16 root directory might not start at a cluster boundary (though it
always ends on one).

However, next_dent() always starts reading the directory at the
beginning of the cluster, causing no files to be found (at least if the
beginning of the cluster contains zeroes).

Fix that by skipping to the actual start of the root directory in such a
case.

This is a relatively common case as at least the filesystem formatter on
Win7 seems to create such filesystems by default on 2GB USB sticks
(cluster size 64 sectors, rootdir size 32 sectors).

dosfstools mkfs.vfat does not seem to create such filesystems.

Signed-off-by: Anssi Hannula <anssi.hannula@bitwise.fi>
---
 fs/fat/fat.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/fs/fat/fat.c b/fs/fat/fat.c
index b08949d370..4a0d4bb8bc 100644
--- a/fs/fat/fat.c
+++ b/fs/fat/fat.c
@@ -782,6 +782,20 @@ static dir_entry *next_dent(fat_itr *itr)
 
 		itr->remaining = nbytes / sizeof(dir_entry) - 1;
 		itr->dent = dent;
+
+		if (itr->fsdata->fatsize != 32 &&
+		    itr->clust == itr->fsdata->root_cluster) {
+			/* rootdir might not start at cluster boundary */
+			u32 sect_offset = itr->fsdata->rootdir_sect -
+				clust_to_sect(itr->fsdata,
+					      itr->fsdata->root_cluster);
+			u32 dirent_offset = itr->fsdata->sect_size /
+					    sizeof(dir_entry) * sect_offset;
+
+			itr->remaining -= dirent_offset;
+			itr->dent += dirent_offset;
+		}
+
 	} else {
 		itr->remaining--;
 		itr->dent++;
-- 
2.17.1

[U-Boot] Antwort: [PATCH] fs: fat: fix reading non-cluster-aligned root directory

[Bernhard Messerklinger]

Hi Anssi,

I tested your patch because i faced the same problem.
But I need an addition to your patch to get everything to work.

Since for fat12/16 the sect_to_clust() calculation is always a negative 
value
the division through the cluster size with an odd negative value cuts the 
rest.
With the next clust_to_sect() call the now even cluster number is 
multiplied 
by the cluster size and and the data_begin section is added. So after the 
calculation without the rest the negative value is smaller and my 
rootdir_sect
is higher then the actual rootdir_sect.
In my case:
clust_size = 2
rootdir_sect = 113
dara_begin = 132

sect_to_clust: 0xfffffff1 = (0x113 - 132) / 2
sect_to_clust: 114 = 132 + 0xfffffff1 * 2

Now my root_cluster is above the root dir but it should be below it (112).
I fixed this with the following patch:

---

 fs/fat/fat.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/fat/fat.c b/fs/fat/fat.c
index de5c7210be..695b6323b1 100644
--- a/fs/fat/fat.c
+++ b/fs/fat/fat.c
@@ -587,7 +587,9 @@ static int get_fs_info(fsdata *mydata)
                                        mydata->rootdir_size -
                                        (mydata->clust_size * 2);
                mydata->root_cluster =
-                       sect_to_clust(mydata, mydata->rootdir_sect);
+                       sect_to_clust(mydata, mydata->rootdir_sect -
+                                     (mydata->rootdir_sect %
+                                      mydata->clust_size));
        }
 
        mydata->fatbufnum = -1;


After patch:
sect_to_clust: 0xfffffff0 = (0x112 - 132) / 2
sect_to_clust: 112 = 132 + 0xfffffff0 * 2

Can you verify this?
If yes? Is it maybe possible to add this to your patch?

Regards,
Bernhard

[U-Boot] [PATCH v2] fs: fat: fix reading non-cluster-aligned root directory

[Anssi Hannula]

A FAT12/FAT16 root directory location is specified by a sector offset and
it might not start at a cluster boundary. It also resides before the
data area (before cluster 2).

However, the current code assumes that the root directory is located at
a beginning of a cluster, causing no files to be found if that is not
the case.

Since the FAT12/FAT16 root directory is located before the data area
and is not aligned to clusters, using unsigned cluster numbers to refer
to the root directory does not work well (the "cluster number" may be
negative, and even allowing it be signed would not make it properly
aligned).

Modify the code to not use the normal cluster numbering when referring to
the root directory of FAT12/FAT16 and instead use a cluster-sized
offsets counted from the root directory start sector.

This is a relatively common case as at least the filesystem formatter on
Win7 seems to create such filesystems by default on 2GB USB sticks when
"FAT" is selected (cluster size 64 sectors, rootdir size 32 sectors,
rootdir starts at half a cluster before cluster 2).

dosfstools mkfs.vfat does not seem to create affected filesystems.

Signed-off-by: Anssi Hannula <anssi.hannula@bitwise.fi>
---

v2: Rewrite to avoid negative "cluster numbers".


Hi,

I'm sorry about not responding in a timely manner.

Bernhard Messerklinger wrote:
> clust_size = 2
> rootdir_sect = 113
> dara_begin = 132
> 
> sect_to_clust: 0xfffffff1 = (0x113 - 132) / 2
> sect_to_clust: 114 = 132 + 0xfffffff1 * 2
> 
> Now my root_cluster is above the root dir but it should be below it (112).

You are right, root_cluster going negative is not being handled properly.

However, I'd rather avoid that in the first place, as the code generally
assumes that cluster numbers are unsigned - which is the reality as well,
it is just that the FAT12/16 rootdir is located before the clusters.

So here is a different take on the original patch that instead avoids
using the "cluster numbers" to refer to the root directory on FAT12/16
altogether.


 fs/fat/fat.c | 47 ++++++++++++++++++++++++++++++++++-------------
 1 file changed, 34 insertions(+), 13 deletions(-)

diff --git a/fs/fat/fat.c b/fs/fat/fat.c
index 6ade4ea54e..c5997c2173 100644
--- a/fs/fat/fat.c
+++ b/fs/fat/fat.c
@@ -602,8 +602,13 @@ static int get_fs_info(fsdata *mydata)
 		mydata->data_begin = mydata->rootdir_sect +
 					mydata->rootdir_size -
 					(mydata->clust_size * 2);
-		mydata->root_cluster =
-			sect_to_clust(mydata, mydata->rootdir_sect);
+
+		/*
+		 * The root directory is not cluster-aligned and may be on a
+		 * "negative" cluster, this will be handled specially in
+		 * next_cluster().
+		 */
+		mydata->root_cluster = 0;
 	}
 
 	mydata->fatbufnum = -1;
@@ -733,20 +738,38 @@ static void fat_itr_child(fat_itr *itr, fat_itr *parent)
 	itr->last_cluster = 0;
 }
 
-static void *next_cluster(fat_itr *itr)
+static void *next_cluster(fat_itr *itr, unsigned *nbytes)
 {
 	fsdata *mydata = itr->fsdata;  /* for silly macros */
 	int ret;
 	u32 sect;
+	u32 read_size;
 
 	/* have we reached the end? */
 	if (itr->last_cluster)
 		return NULL;
 
-	sect = clust_to_sect(itr->fsdata, itr->next_clust);
+	if (itr->is_root && itr->fsdata->fatsize != 32) {
+		/*
+		 * The root directory is located before the data area and
+		 * cannot be indexed using the regular unsigned cluster
+		 * numbers (it may start at a "negative" cluster or not at a
+		 * cluster boundary at all), so consider itr->next_clust to be
+		 * a offset in cluster-sized units from the start of rootdir.
+		 */
+		unsigned sect_offset = itr->next_clust * itr->fsdata->clust_size;
+		unsigned remaining_sects = itr->fsdata->rootdir_size - sect_offset;
+		sect = itr->fsdata->rootdir_sect + sect_offset;
+		/* do not read past the end of rootdir */
+		read_size = min_t(u32, itr->fsdata->clust_size,
+				  remaining_sects);
+	} else {
+		sect = clust_to_sect(itr->fsdata, itr->next_clust);
+		read_size = itr->fsdata->clust_size;
+	}
 
-	debug("FAT read(sect=%d), clust_size=%d, DIRENTSPERBLOCK=%zd\n",
-	      sect, itr->fsdata->clust_size, DIRENTSPERBLOCK);
+	debug("FAT read(sect=%d), clust_size=%d, read_size=%u, DIRENTSPERBLOCK=%zd\n",
+	      sect, itr->fsdata->clust_size, read_size, DIRENTSPERBLOCK);
 
 	/*
 	 * NOTE: do_fat_read_at() had complicated logic to deal w/
@@ -757,18 +780,17 @@ static void *next_cluster(fat_itr *itr)
 	 * dent at a time and iteratively constructing the vfat long
 	 * name.
 	 */
-	ret = disk_read(sect, itr->fsdata->clust_size,
-			itr->block);
+	ret = disk_read(sect, read_size, itr->block);
 	if (ret < 0) {
 		debug("Error: reading block\n");
 		return NULL;
 	}
 
+	*nbytes = read_size * itr->fsdata->sect_size;
 	itr->clust = itr->next_clust;
 	if (itr->is_root && itr->fsdata->fatsize != 32) {
 		itr->next_clust++;
-		sect = clust_to_sect(itr->fsdata, itr->next_clust);
-		if (sect - itr->fsdata->rootdir_sect >=
+		if (itr->next_clust * itr->fsdata->clust_size >=
 		    itr->fsdata->rootdir_size) {
 			debug("nextclust: 0x%x\n", itr->next_clust);
 			itr->last_cluster = 1;
@@ -787,9 +809,8 @@ static void *next_cluster(fat_itr *itr)
 static dir_entry *next_dent(fat_itr *itr)
 {
 	if (itr->remaining == 0) {
-		struct dir_entry *dent = next_cluster(itr);
-		unsigned nbytes = itr->fsdata->sect_size *
-			itr->fsdata->clust_size;
+		unsigned nbytes;
+		struct dir_entry *dent = next_cluster(itr, &nbytes);
 
 		/* have we reached the last cluster? */
 		if (!dent) {
-- 
Anssi Hannula / Bitwise Oy

[U-Boot] Antwort: [PATCH v2] fs: fat: fix reading non-cluster-aligned root directory

[Bernhard Messerklinger]

Reviewed-by: Bernhard Messerklinger 
<bernhard.messerklinger@br-automation.com>
Tested-by: Bernhard Messerklinger 
<bernhard.messerklinger@br-automation.com>



Von:    Anssi Hannula <anssi.hannula@bitwise.fi>
An:     u-boot at lists.denx.de, Bernhard Messerklinger 
<bernhard.messerklinger@br-automation.com>
Kopie:  Hannes Schmelzer <Hannes.Schmelzer@br-automation.com>
Datum:  02/27/2019 11:56 AM
Betreff:        [PATCH v2] fs: fat: fix reading non-cluster-aligned root 
directory



A FAT12/FAT16 root directory location is specified by a sector offset and
it might not start at a cluster boundary. It also resides before the
data area (before cluster 2).

However, the current code assumes that the root directory is located at
a beginning of a cluster, causing no files to be found if that is not
the case.

Since the FAT12/FAT16 root directory is located before the data area
and is not aligned to clusters, using unsigned cluster numbers to refer
to the root directory does not work well (the "cluster number" may be
negative, and even allowing it be signed would not make it properly
aligned).

Modify the code to not use the normal cluster numbering when referring to
the root directory of FAT12/FAT16 and instead use a cluster-sized
offsets counted from the root directory start sector.

This is a relatively common case as at least the filesystem formatter on
Win7 seems to create such filesystems by default on 2GB USB sticks when
"FAT" is selected (cluster size 64 sectors, rootdir size 32 sectors,
rootdir starts at half a cluster before cluster 2).

dosfstools mkfs.vfat does not seem to create affected filesystems.

Signed-off-by: Anssi Hannula <anssi.hannula@bitwise.fi>
---

v2: Rewrite to avoid negative "cluster numbers".


Hi,

I'm sorry about not responding in a timely manner.

Bernhard Messerklinger wrote:
> clust_size = 2
> rootdir_sect = 113
> dara_begin = 132
> 
> sect_to_clust: 0xfffffff1 = (0x113 - 132) / 2
> sect_to_clust: 114 = 132 + 0xfffffff1 * 2
> 
> Now my root_cluster is above the root dir but it should be below it 
(112).

You are right, root_cluster going negative is not being handled properly.

However, I'd rather avoid that in the first place, as the code generally
assumes that cluster numbers are unsigned - which is the reality as well,
it is just that the FAT12/16 rootdir is located before the clusters.

So here is a different take on the original patch that instead avoids
using the "cluster numbers" to refer to the root directory on FAT12/16
altogether.


 fs/fat/fat.c | 47 ++++++++++++++++++++++++++++++++++-------------
 1 file changed, 34 insertions(+), 13 deletions(-)

diff --git a/fs/fat/fat.c b/fs/fat/fat.c
index 6ade4ea54e..c5997c2173 100644
--- a/fs/fat/fat.c
+++ b/fs/fat/fat.c
@@ -602,8 +602,13 @@ static int get_fs_info(fsdata *mydata)
                                 mydata->data_begin = mydata->rootdir_sect 
+
  mydata->rootdir_size -
  (mydata->clust_size * 2);
-                                mydata->root_cluster =
-                                                sect_to_clust(mydata, 
mydata->rootdir_sect);
+
+                                /*
+                                 * The root directory is not 
cluster-aligned and may be on a
+                                 * "negative" cluster, this will be 
handled specially in
+                                 * next_cluster().
+                                 */
+                                mydata->root_cluster = 0;
                 }
 
                 mydata->fatbufnum = -1;
@@ -733,20 +738,38 @@ static void fat_itr_child(fat_itr *itr, fat_itr 
*parent)
                 itr->last_cluster = 0;
 }
 
-static void *next_cluster(fat_itr *itr)
+static void *next_cluster(fat_itr *itr, unsigned *nbytes)
 {
                 fsdata *mydata = itr->fsdata;  /* for silly macros */
                 int ret;
                 u32 sect;
+                u32 read_size;
 
                 /* have we reached the end? */
                 if (itr->last_cluster)
                                 return NULL;
 
-                sect = clust_to_sect(itr->fsdata, itr->next_clust);
+                if (itr->is_root && itr->fsdata->fatsize != 32) {
+                                /*
+                                 * The root directory is located before 
the data area and
+                                 * cannot be indexed using the regular 
unsigned cluster
+                                 * numbers (it may start at a "negative" 
cluster or not at a
+                                 * cluster boundary at all), so consider 
itr->next_clust to be
+                                 * a offset in cluster-sized units from 
the start of rootdir.
+                                 */
+                                unsigned sect_offset = itr->next_clust * 
itr->fsdata->clust_size;
+                                unsigned remaining_sects = 
itr->fsdata->rootdir_size - sect_offset;
+                                sect = itr->fsdata->rootdir_sect + 
sect_offset;
+                                /* do not read past the end of rootdir */
+                                read_size = min_t(u32, 
itr->fsdata->clust_size,
+ remaining_sects);
+                } else {
+                                sect = clust_to_sect(itr->fsdata, 
itr->next_clust);
+                                read_size = itr->fsdata->clust_size;
+                }
 
-                debug("FAT read(sect=%d), clust_size=%d, 
DIRENTSPERBLOCK=%zd\n",
-                      sect, itr->fsdata->clust_size, DIRENTSPERBLOCK);
+                debug("FAT read(sect=%d), clust_size=%d, read_size=%u, 
DIRENTSPERBLOCK=%zd\n",
+                      sect, itr->fsdata->clust_size, read_size, 
DIRENTSPERBLOCK);
 
                 /*
                  * NOTE: do_fat_read_at() had complicated logic to deal 
w/
@@ -757,18 +780,17 @@ static void *next_cluster(fat_itr *itr)
                  * dent at a time and iteratively constructing the vfat 
long
                  * name.
                  */
-                ret = disk_read(sect, itr->fsdata->clust_size,
-                                                itr->block);
+                ret = disk_read(sect, read_size, itr->block);
                 if (ret < 0) {
                                 debug("Error: reading block\n");
                                 return NULL;
                 }
 
+                *nbytes = read_size * itr->fsdata->sect_size;
                 itr->clust = itr->next_clust;
                 if (itr->is_root && itr->fsdata->fatsize != 32) {
                                 itr->next_clust++;
-                                sect = clust_to_sect(itr->fsdata, 
itr->next_clust);
-                                if (sect - itr->fsdata->rootdir_sect >=
+                                if (itr->next_clust * 
itr->fsdata->clust_size >=
                                     itr->fsdata->rootdir_size) {
                                                 debug("nextclust: 
0x%x\n", itr->next_clust);
                                                 itr->last_cluster = 1;
@@ -787,9 +809,8 @@ static void *next_cluster(fat_itr *itr)
 static dir_entry *next_dent(fat_itr *itr)
 {
                 if (itr->remaining == 0) {
-                                struct dir_entry *dent = 
next_cluster(itr);
-                                unsigned nbytes = itr->fsdata->sect_size 
*
-                                                itr->fsdata->clust_size;
+                                unsigned nbytes;
+                                struct dir_entry *dent = 
next_cluster(itr, &nbytes);
 
                                 /* have we reached the last cluster? */
                                 if (!dent) {
-- 
Anssi Hannula / Bitwise Oy

[U-Boot] [U-Boot, v2] fs: fat: fix reading non-cluster-aligned root directory

[Tom Rini]

On Wed, Feb 27, 2019 at 12:55:57PM +0200, Anssi Hannula wrote:

> A FAT12/FAT16 root directory location is specified by a sector offset and
> it might not start at a cluster boundary. It also resides before the
> data area (before cluster 2).
> 
> However, the current code assumes that the root directory is located at
> a beginning of a cluster, causing no files to be found if that is not
> the case.
> 
> Since the FAT12/FAT16 root directory is located before the data area
> and is not aligned to clusters, using unsigned cluster numbers to refer
> to the root directory does not work well (the "cluster number" may be
> negative, and even allowing it be signed would not make it properly
> aligned).
> 
> Modify the code to not use the normal cluster numbering when referring to
> the root directory of FAT12/FAT16 and instead use a cluster-sized
> offsets counted from the root directory start sector.
> 
> This is a relatively common case as at least the filesystem formatter on
> Win7 seems to create such filesystems by default on 2GB USB sticks when
> "FAT" is selected (cluster size 64 sectors, rootdir size 32 sectors,
> rootdir starts at half a cluster before cluster 2).
> 
> dosfstools mkfs.vfat does not seem to create affected filesystems.
> 
> Signed-off-by: Anssi Hannula <anssi.hannula@bitwise.fi>
> Reviewed-by: Bernhard Messerklinger <bernhard.messerklinger@br-automation.com>
> Tested-by: Bernhard Messerklinger <bernhard.messerklinger@br-automation.com>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190410/5d90ca25/attachment.sig>