mkfs.xfs: null-terminate symlinks created via protofile

PATCH In review Eric Sandeen 3 participants 9 messages Nov 27, 2018 – Dec 12, 2018

Share & tools

Eric Sandeen

author

2018-11-27 09:35 UTC #0 lore raw

PATCH 1/1 mkfs.xfs: null-terminate symlinks created via protofile

Now that we have a symlink verifier which checks that in-memory symlink names are null-terminated, be sure we do that when we create them via the mkfs protofile.

We only want to null-terminate inline data if it's a symlink; we only ever /call/ newfile() with "dolocal" for symlinks, so rename that function argument for clarity.

Zorro found this by running xfs/019 on an s390x machine, it failed with:

Metadata corruption detected at 0x101214a, inode 0x89 data fork

Signed-off-by Eric Sandeen <[email protected]>

Reported-by Zorro Lang <[email protected]>

---

mkfs/proto.c +5 -3

index 1cd5436..d76c80d 100644
@@ -15,7 +15,7 @@ static char *getstr(char **pp);
 static void fail(char *msg, int i);
 static struct xfs_trans * getres(struct xfs_mount *mp, uint blocks);
 static void rsvfile(xfs_mount_t *mp, xfs_inode_t *ip, long long len);
-static int newfile(xfs_trans_t *tp, xfs_inode_t *ip, int dolocal, int logit,
+static int newfile(xfs_trans_t *tp, xfs_inode_t *ip, int symlink, int logit,
 			char *buf, int len);
 static char *newregfile(char **pp, int *len);
 static void rtinit(xfs_mount_t *mp);

@@ -220,7 +220,7 @@ static int
 newfile(
 	xfs_trans_t	*tp,
 	xfs_inode_t	*ip,
-	int		dolocal,
+	int		symlink,
 	int		logit,
 	char		*buf,
 	int		len)

@@ -236,7 +236,9 @@ newfile(
 
 	flags = 0;
 	mp = ip->i_mount;
-	if (dolocal && len <= XFS_IFORK_DSIZE(ip)) {
+	if (symlink && len <= XFS_IFORK_DSIZE(ip)) {
+		/* Copy the name's trailing NULL as well */
+		len += 1;
 		libxfs_idata_realloc(ip, len, XFS_DATA_FORK);
 		if (buf)
 			memmove(ip->i_df.if_u1.if_data, buf, len);

Darrick J. Wong

2018-11-27 09:51 UTC #1 lore raw

Quoted 40 lines · Eric Sandeen

Now that we have a symlink verifier which checks that in-memory symlink names are null-terminated, be sure we do that when we create them via the mkfs protofile.
We only want to null-terminate inline data if it's a symlink; we only ever /call/ newfile() with "dolocal" for symlinks, so rename that function argument for clarity.
Zorro found this by running xfs/019 on an s390x machine, it failed with:
Metadata corruption detected at 0x101214a, inode 0x89 data fork
Signed-off-by Eric Sandeen <[email protected]>
Reported-by Zorro Lang <[email protected]>
---
mkfs/proto.c +2 -2
index 1cd5436..d76c80d 100644
@@ -15,7 +15,7 @@ static char *getstr(char **pp);
 static void fail(char *msg, int i);
 static struct xfs_trans * getres(struct xfs_mount *mp, uint blocks);
 static void rsvfile(xfs_mount_t *mp, xfs_inode_t *ip, long long len);
-static int newfile(xfs_trans_t *tp, xfs_inode_t *ip, int dolocal, int logit,
+static int newfile(xfs_trans_t *tp, xfs_inode_t *ip, int symlink, int logit,
 			char *buf, int len);
 static char *newregfile(char **pp, int *len);
 static void rtinit(xfs_mount_t *mp);
@@ -220,7 +220,7 @@ static int
 newfile(
 	xfs_trans_t	*tp,
 	xfs_inode_t	*ip,
-	int		dolocal,
+	int		symlink,
 	int		logit,
 	char		*buf,
 	int		len)

mkfs/proto.c newfile( · line 241

Quoted 7 lines

mkfs/proto.c +3 -1

@@ -236,7 +236,9 @@ newfile(
 
 	flags = 0;
 	mp = ip->i_mount;
-	if (dolocal && len <= XFS_IFORK_DSIZE(ip)) {
+	if (symlink && len <= XFS_IFORK_DSIZE(ip)) {
+		/* Copy the name's trailing NULL as well */
+		len += 1;

We set di_size to len later in this function, which means that we now write out a symlink with a size larger than the symlink target, right?

--D

Quoted 4 lines

mkfs/proto.c +0 -0

 		libxfs_idata_realloc(ip, len, XFS_DATA_FORK);
 		if (buf)
 			memmove(ip->i_df.if_u1.if_data, buf, len);

Eric Sandeen

2018-11-27 09:56 UTC #2 lore raw

Quoted 42 lines · Darrick J. Wong

Quoted 41 lines · Eric Sandeen
Now that we have a symlink verifier which checks that in-memory symlink names are null-terminated, be sure we do that when we create them via the mkfs protofile.
We only want to null-terminate inline data if it's a symlink; we only ever /call/ newfile() with "dolocal" for symlinks, so rename that function argument for clarity.
Zorro found this by running xfs/019 on an s390x machine, it failed with:
Metadata corruption detected at 0x101214a, inode 0x89 data fork
Signed-off-by Eric Sandeen <[email protected]>
Reported-by Zorro Lang <[email protected]>
---
mkfs/proto.c +2 -2
index 1cd5436..d76c80d 100644
@@ -15,7 +15,7 @@ static char *getstr(char **pp);
 static void fail(char *msg, int i);
 static struct xfs_trans * getres(struct xfs_mount *mp, uint blocks);
 static void rsvfile(xfs_mount_t *mp, xfs_inode_t *ip, long long len);
-static int newfile(xfs_trans_t *tp, xfs_inode_t *ip, int dolocal, int logit,
+static int newfile(xfs_trans_t *tp, xfs_inode_t *ip, int symlink, int logit,
 			char *buf, int len);
 static char *newregfile(char **pp, int *len);
 static void rtinit(xfs_mount_t *mp);
@@ -220,7 +220,7 @@ static int
 newfile(
 	xfs_trans_t	*tp,
 	xfs_inode_t	*ip,
-	int		dolocal,
+	int		symlink,
 	int		logit,
 	char		*buf,
 	int		len)
@@ -236,7 +236,9 @@ newfile(
 

Quoted 9 lines

mkfs/proto.c newfile( · line 241
Quoted 6 lines
mkfs/proto.c +3 -1
 	flags = 0;
 	mp = ip->i_mount;
-	if (dolocal && len <= XFS_IFORK_DSIZE(ip)) {
+	if (symlink && len <= XFS_IFORK_DSIZE(ip)) {
+		/* Copy the name's trailing NULL as well */
+		len += 1;
We set di_size to len later in this function, which means that we now write out a symlink with a size larger than the symlink target, right?

sonova...

Quoted 7 lines

--D

Quoted 4 lines

mkfs/proto.c +0 -0

 		libxfs_idata_realloc(ip, len, XFS_DATA_FORK);
 		if (buf)
 			memmove(ip->i_df.if_u1.if_data, buf, len);

Eric Sandeen

2018-11-27 10:00 UTC #3 lore raw

PATCH v2 1/1 mkfs.xfs: null-terminate symlinks created via protofile

In reply to Eric Sandeen

Now that we have a symlink verifier which checks that in-memory symlink names are null-terminated, be sure we do that when we create them via the mkfs protofile.

We only want to null-terminate inline data if it's a symlink; we only ever /call/ newfile() with "dolocal" for symlinks, so rename that function argument for clarity.

Zorro found this by running xfs/019 on an s390x machine, it failed with:

Metadata corruption detected at 0x101214a, inode 0x89 data fork

Signed-off-by Eric Sandeen <[email protected]>

Reported-by Zorro Lang <[email protected]>

---

V2: don't increase len, it gets set into i_size. Just bump the allocation/copy by 1. Thanks, Darrick.

mkfs/proto.c +6 -5

index 1cd5436..103795f 100644
@@ -15,7 +15,7 @@ static char *getstr(char **pp);
 static void fail(char *msg, int i);
 static struct xfs_trans * getres(struct xfs_mount *mp, uint blocks);
 static void rsvfile(xfs_mount_t *mp, xfs_inode_t *ip, long long len);
-static int newfile(xfs_trans_t *tp, xfs_inode_t *ip, int dolocal, int logit,
+static int newfile(xfs_trans_t *tp, xfs_inode_t *ip, int symlink, int logit,
 			char *buf, int len);
 static char *newregfile(char **pp, int *len);
 static void rtinit(xfs_mount_t *mp);

@@ -220,7 +220,7 @@ static int
 newfile(
 	xfs_trans_t	*tp,
 	xfs_inode_t	*ip,
-	int		dolocal,
+	int		symlink,
 	int		logit,
 	char		*buf,
 	int		len)

@@ -236,10 +236,11 @@ newfile(
 
 	flags = 0;
 	mp = ip->i_mount;
-	if (dolocal && len <= XFS_IFORK_DSIZE(ip)) {
-		libxfs_idata_realloc(ip, len, XFS_DATA_FORK);
+	if (symlink && len <= XFS_IFORK_DSIZE(ip)) {
+		/* Copy the name's trailing NULL as well */
+		libxfs_idata_realloc(ip, len + 1, XFS_DATA_FORK);
 		if (buf)
-			memmove(ip->i_df.if_u1.if_data, buf, len);
+			memmove(ip->i_df.if_u1.if_data, buf, len + 1);
 		ip->i_d.di_size = len;
 		ip->i_df.if_flags &= ~XFS_IFEXTENTS;
 		ip->i_df.if_flags |= XFS_IFINLINE;

Darrick J. Wong

Reviewed-by on v2 1/1

2018-11-28 03:16 UTC #4 lore raw

Quoted 8 lines · Eric Sandeen

Now that we have a symlink verifier which checks that in-memory symlink names are null-terminated, be sure we do that when we create them via the mkfs protofile.
We only want to null-terminate inline data if it's a symlink; we only ever /call/ newfile() with "dolocal" for symlinks, so rename that function argument for clarity.

Quoted 7 lines

Zorro found this by running xfs/019 on an s390x machine, it failed with:
Metadata corruption detected at 0x101214a, inode 0x89 data fork
Signed-off-by Eric Sandeen <[email protected]>
Reported-by Zorro Lang <[email protected]>

Looks ok, though /me wonders if we really should be collecting these higher level routines in a library or something... but that's a wider-ranging cleanup for another time.

Reviewed-by Darrick J. Wong <[email protected]>

--D

Quoted 43 lines

---

V2: don't increase len, it gets set into i_size. Just bump the allocation/copy by 1. Thanks, Darrick.

mkfs/proto.c +6 -5

index 1cd5436..103795f 100644
@@ -15,7 +15,7 @@ static char *getstr(char **pp);
 static void fail(char *msg, int i);
 static struct xfs_trans * getres(struct xfs_mount *mp, uint blocks);
 static void rsvfile(xfs_mount_t *mp, xfs_inode_t *ip, long long len);
-static int newfile(xfs_trans_t *tp, xfs_inode_t *ip, int dolocal, int logit,
+static int newfile(xfs_trans_t *tp, xfs_inode_t *ip, int symlink, int logit,
 			char *buf, int len);
 static char *newregfile(char **pp, int *len);
 static void rtinit(xfs_mount_t *mp);

@@ -220,7 +220,7 @@ static int
 newfile(
 	xfs_trans_t	*tp,
 	xfs_inode_t	*ip,
-	int		dolocal,
+	int		symlink,
 	int		logit,
 	char		*buf,
 	int		len)

@@ -236,10 +236,11 @@ newfile(
 
 	flags = 0;
 	mp = ip->i_mount;
-	if (dolocal && len <= XFS_IFORK_DSIZE(ip)) {
-		libxfs_idata_realloc(ip, len, XFS_DATA_FORK);
+	if (symlink && len <= XFS_IFORK_DSIZE(ip)) {
+		/* Copy the name's trailing NULL as well */
+		libxfs_idata_realloc(ip, len + 1, XFS_DATA_FORK);
 		if (buf)
-			memmove(ip->i_df.if_u1.if_data, buf, len);
+			memmove(ip->i_df.if_u1.if_data, buf, len + 1);
 		ip->i_d.di_size = len;
 		ip->i_df.if_flags &= ~XFS_IFEXTENTS;
 		ip->i_df.if_flags |= XFS_IFINLINE;

Darrick J. Wong

2018-12-11 19:03 UTC #5 lore raw

PATCH 1/1 mkfs: fix symlink target if_bytes computation for protofile

In reply to Eric Sandeen

From: Darrick J. Wong <[email protected]>

When creating a local format symlink, we expect the target buffer in the data fork to have enough space to contain the null, but we also expect if_bytes to reflect the length of the target /not/ including the null. If we don't adjust if_bytes down by one byte, we can run off into uninitialized memory. Fix this, which should clean up the spurious xfs/019 failures for good.

Signed-off-by Darrick J. Wong <[email protected]>

---
 mkfs/proto.c |   10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

mkfs/proto.c +9 -1

index fc07de5f..dc0225bd 100644
@@ -238,10 +238,18 @@ newfile(
 	flags = 0;
 	mp = ip->i_mount;
 	if (symlink && len <= XFS_IFORK_DSIZE(ip)) {
-		/* Copy the name's trailing NULL as well */
+		/*
+		 * Local format symbolic link targets are supposed to be NULL
+		 * terminated in memory.  This means that if_data must be at
+		 * least one byte longer than the target string's length so
+		 * that there's enough space to hold the null.  However, we
+		 * still expect if_bytes to be strlen(target), which does _not_
+		 * include the null.
+		 */
 		libxfs_idata_realloc(ip, len + 1, XFS_DATA_FORK);
 		if (buf)
 			memmove(ip->i_df.if_u1.if_data, buf, len + 1);
+		ip->i_df.if_bytes = len;
 		ip->i_d.di_size = len;
 		ip->i_df.if_flags &= ~XFS_IFEXTENTS;
 		ip->i_df.if_flags |= XFS_IFINLINE;

Eric Sandeen

2018-12-11 20:36 UTC #6 lore raw

Re: [PATCH] mkfs: fix symlink target if_bytes computation for protofile

Quoted 10 lines · Darrick J. Wong

From: Darrick J. Wong <[email protected]>
When creating a local format symlink, we expect the target buffer in the data fork to have enough space to contain the null, but we also expect if_bytes to reflect the length of the target /not/ including the null. If we don't adjust if_bytes down by one byte, we can run off into uninitialized memory. Fix this, which should clean up the spurious xfs/019 failures for good.
Signed-off-by Darrick J. Wong <[email protected]>

Ugh. Why do we even have protofiles? I have never seen these in use in real life. Can we just deprecate this half-baked crap? :/

You suggested on IRC that we just use xfs_init_local fork; I had looked at that when I did the first patch and for some reason didn't use it, but looking again it seems ... ok. I hate how all of this stuff is re-implemented in the protofile crap code and it seems like we can't re-use too many of the libxfs functions directly but may as well use this if we can, right?

=========

mkfs: don't open code local fork setup in protofile code

Signed-off-by Eric Sandeen <[email protected]>

---

libxfs/libxfs_api_defs.h +1 -0

index ad5d65de..c3792e12 100644
@@ -149,5 +149,6 @@
 #define xfs_dir_get_ops			libxfs_dir_get_ops
 #define xfs_default_ifork_ops		libxfs_default_ifork_ops
 #define xfs_fs_geometry			libxfs_fs_geometry
+#define xfs_init_local_fork		libxfs_init_local_fork
 
 #endif /* __LIBXFS_API_DEFS_H__ */

mkfs/proto.c +1 -7

index 103795f1..3bba4917 100644
@@ -237,13 +237,7 @@ newfile(
 	flags = 0;
 	mp = ip->i_mount;
 	if (symlink && len <= XFS_IFORK_DSIZE(ip)) {
-		/* Copy the name's trailing NULL as well */
-		libxfs_idata_realloc(ip, len + 1, XFS_DATA_FORK);
-		if (buf)
-			memmove(ip->i_df.if_u1.if_data, buf, len + 1);
-		ip->i_d.di_size = len;
-		ip->i_df.if_flags &= ~XFS_IFEXTENTS;
-		ip->i_df.if_flags |= XFS_IFINLINE;
+		libxfs_init_local_fork(ip, XFS_DATA_FORK, buf, len);
 		ip->i_d.di_format = XFS_DINODE_FMT_LOCAL;
 		flags = XFS_ILOG_DDATA;
 	} else if (len > 0) {

Darrick J. Wong

2018-12-11 22:42 UTC #7 lore raw

Re: [PATCH] mkfs: fix symlink target if_bytes computation for protofile

Quoted 10 lines · Eric Sandeen

Quoted 9 lines · Darrick J. Wong
From: Darrick J. Wong <[email protected]>
When creating a local format symlink, we expect the target buffer in the data fork to have enough space to contain the null, but we also expect if_bytes to reflect the length of the target /not/ including the null. If we don't adjust if_bytes down by one byte, we can run off into uninitialized memory. Fix this, which should clean up the spurious xfs/019 failures for good.

Quoted 7 lines

Quoted 1 line
Signed-off-by Darrick J. Wong <[email protected]>
Ugh. Why do we even have protofiles? I have never seen these in use in real life. Can we just deprecate this half-baked crap? :/
You suggested on IRC that we just use xfs_init_local fork; I had looked at that when I did the first patch and for some reason didn't use it,

Admittedly I wondered if we can leak memory that way, but AFAICT a new inode shouldn't really have if_bytes > 0, right?

Quoted 4 lines

but looking again it seems ... ok. I hate how all of this stuff is re-implemented in the protofile crap code and it seems like we can't re-use too many of the libxfs functions directly but may as well use this if we can, right?

Funny you mention it, but I've been quietly refactoring the xfs_inode.c code into libxfs as part of preparing for the metadata directory feature, which has enabled me to cut a considerable amount of opencoded crap out of mkfs and repair. Granted, I still have to make sure it all /works/, but ... yes this all should be using libxfs functions. :)

Uh, I'll give this patch a spin and see what happens.

--D

Quoted 38 lines

=========

mkfs: don't open code local fork setup in protofile code

Signed-off-by Eric Sandeen <[email protected]>

---

libxfs/libxfs_api_defs.h +1 -0

index ad5d65de..c3792e12 100644
@@ -149,5 +149,6 @@
 #define xfs_dir_get_ops			libxfs_dir_get_ops
 #define xfs_default_ifork_ops		libxfs_default_ifork_ops
 #define xfs_fs_geometry			libxfs_fs_geometry
+#define xfs_init_local_fork		libxfs_init_local_fork
 
 #endif /* __LIBXFS_API_DEFS_H__ */

mkfs/proto.c +1 -7

index 103795f1..3bba4917 100644
@@ -237,13 +237,7 @@ newfile(
 	flags = 0;
 	mp = ip->i_mount;
 	if (symlink && len <= XFS_IFORK_DSIZE(ip)) {
-		/* Copy the name's trailing NULL as well */
-		libxfs_idata_realloc(ip, len + 1, XFS_DATA_FORK);
-		if (buf)
-			memmove(ip->i_df.if_u1.if_data, buf, len + 1);
-		ip->i_d.di_size = len;
-		ip->i_df.if_flags &= ~XFS_IFEXTENTS;
-		ip->i_df.if_flags |= XFS_IFINLINE;
+		libxfs_init_local_fork(ip, XFS_DATA_FORK, buf, len);
 		ip->i_d.di_format = XFS_DINODE_FMT_LOCAL;
 		flags = XFS_ILOG_DDATA;
 	} else if (len > 0) {

Darrick J. Wong

Reviewed-by

2018-12-12 04:50 UTC #8 lore raw

Re: [PATCH] mkfs: fix symlink target if_bytes computation for protofile

Quoted 27 lines · Darrick J. Wong

Quoted 10 lines · Eric Sandeen
Quoted 9 lines · Darrick J. Wong
From: Darrick J. Wong <[email protected]>
When creating a local format symlink, we expect the target buffer in the data fork to have enough space to contain the null, but we also expect if_bytes to reflect the length of the target /not/ including the null. If we don't adjust if_bytes down by one byte, we can run off into uninitialized memory. Fix this, which should clean up the spurious xfs/019 failures for good.
Quoted 7 lines
Quoted 1 line
Signed-off-by Darrick J. Wong <[email protected]>
Ugh. Why do we even have protofiles? I have never seen these in use in real life. Can we just deprecate this half-baked crap? :/
You suggested on IRC that we just use xfs_init_local fork; I had looked at that when I did the first patch and for some reason didn't use it,
Admittedly I wondered if we can leak memory that way, but AFAICT a new inode shouldn't really have if_bytes > 0, right?
Quoted 4 lines
but looking again it seems ... ok. I hate how all of this stuff is re-implemented in the protofile crap code and it seems like we can't re-use too many of the libxfs functions directly but may as well use this if we can, right?

Quoted 7 lines

Funny you mention it, but I've been quietly refactoring the xfs_inode.c code into libxfs as part of preparing for the metadata directory feature, which has enabled me to cut a considerable amount of opencoded crap out of mkfs and repair. Granted, I still have to make sure it all /works/, but ... yes this all should be using libxfs functions. :)
Uh, I'll give this patch a spin and see what happens.

Seems fine to me, so...

Reviewed-by Darrick J. Wong <[email protected]>

--D

Quoted 41 lines

--D

Quoted 38 lines

=========

mkfs: don't open code local fork setup in protofile code

Signed-off-by Eric Sandeen <[email protected]>

---

libxfs/libxfs_api_defs.h +1 -0

index ad5d65de..c3792e12 100644
@@ -149,5 +149,6 @@
 #define xfs_dir_get_ops			libxfs_dir_get_ops
 #define xfs_default_ifork_ops		libxfs_default_ifork_ops
 #define xfs_fs_geometry			libxfs_fs_geometry
+#define xfs_init_local_fork		libxfs_init_local_fork
 
 #endif /* __LIBXFS_API_DEFS_H__ */

mkfs/proto.c +1 -7

index 103795f1..3bba4917 100644
@@ -237,13 +237,7 @@ newfile(
 	flags = 0;
 	mp = ip->i_mount;
 	if (symlink && len <= XFS_IFORK_DSIZE(ip)) {
-		/* Copy the name's trailing NULL as well */
-		libxfs_idata_realloc(ip, len + 1, XFS_DATA_FORK);
-		if (buf)
-			memmove(ip->i_df.if_u1.if_data, buf, len + 1);
-		ip->i_d.di_size = len;
-		ip->i_df.if_flags &= ~XFS_IFEXTENTS;
-		ip->i_df.if_flags |= XFS_IFINLINE;
+		libxfs_init_local_fork(ip, XFS_DATA_FORK, buf, len);
 		ip->i_d.di_format = XFS_DINODE_FMT_LOCAL;
 		flags = XFS_ILOG_DDATA;
 	} else if (len > 0) {