All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] mkfs.xfs: null-terminate symlinks created via protofile
@ 2018-11-26 22:39 Eric Sandeen
  2018-11-26 22:55 ` Darrick J. Wong
  2018-11-26 23:04 ` [PATCH V2] " Eric Sandeen
  0 siblings, 2 replies; 9+ messages in thread
From: Eric Sandeen @ 2018-11-26 22:39 UTC (permalink / raw)
  To: linux-xfs; +Cc: Zorro Lang

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 <sandeen@redhat.com>
Reported-by: Zorro Lang <zlang@redhat.com>
---

diff --git a/mkfs/proto.c b/mkfs/proto.c
index 1cd5436..d76c80d 100644
--- a/mkfs/proto.c
+++ b/mkfs/proto.c
@@ -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);

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH] mkfs.xfs: null-terminate symlinks created via protofile
  2018-11-26 22:39 [PATCH] mkfs.xfs: null-terminate symlinks created via protofile Eric Sandeen
@ 2018-11-26 22:55 ` Darrick J. Wong
  2018-11-26 23:01   ` Eric Sandeen
  2018-11-26 23:04 ` [PATCH V2] " Eric Sandeen
  1 sibling, 1 reply; 9+ messages in thread
From: Darrick J. Wong @ 2018-11-26 22:55 UTC (permalink / raw)
  To: Eric Sandeen; +Cc: linux-xfs, Zorro Lang

On Mon, Nov 26, 2018 at 04:39:30PM -0600, Eric Sandeen wrote:
> 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 <sandeen@redhat.com>
> Reported-by: Zorro Lang <zlang@redhat.com>
> ---
> 
> diff --git a/mkfs/proto.c b/mkfs/proto.c
> index 1cd5436..d76c80d 100644
> --- a/mkfs/proto.c
> +++ b/mkfs/proto.c
> @@ -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;

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

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] mkfs.xfs: null-terminate symlinks created via protofile
  2018-11-26 22:55 ` Darrick J. Wong
@ 2018-11-26 23:01   ` Eric Sandeen
  0 siblings, 0 replies; 9+ messages in thread
From: Eric Sandeen @ 2018-11-26 23:01 UTC (permalink / raw)
  To: Darrick J. Wong, Eric Sandeen; +Cc: linux-xfs, Zorro Lang

On 11/26/18 4:55 PM, Darrick J. Wong wrote:
> On Mon, Nov 26, 2018 at 04:39:30PM -0600, Eric Sandeen wrote:
>> 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 <sandeen@redhat.com>
>> Reported-by: Zorro Lang <zlang@redhat.com>
>> ---
>>
>> diff --git a/mkfs/proto.c b/mkfs/proto.c
>> index 1cd5436..d76c80d 100644
>> --- a/mkfs/proto.c
>> +++ b/mkfs/proto.c
>> @@ -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;
> 
> 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...

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH V2] mkfs.xfs: null-terminate symlinks created via protofile
  2018-11-26 22:39 [PATCH] mkfs.xfs: null-terminate symlinks created via protofile Eric Sandeen
  2018-11-26 22:55 ` Darrick J. Wong
@ 2018-11-26 23:04 ` Eric Sandeen
  2018-11-27 16:18   ` Darrick J. Wong
  2018-12-11 19:02   ` [PATCH] mkfs: fix symlink target if_bytes computation for protofile Darrick J. Wong
  1 sibling, 2 replies; 9+ messages in thread
From: Eric Sandeen @ 2018-11-26 23:04 UTC (permalink / raw)
  To: Eric Sandeen, linux-xfs; +Cc: Zorro Lang

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 <sandeen@redhat.com>
Reported-by: Zorro Lang <zlang@redhat.com>
---

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

diff --git a/mkfs/proto.c b/mkfs/proto.c
index 1cd5436..103795f 100644
--- a/mkfs/proto.c
+++ b/mkfs/proto.c
@@ -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;

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH V2] mkfs.xfs: null-terminate symlinks created via protofile
  2018-11-26 23:04 ` [PATCH V2] " Eric Sandeen
@ 2018-11-27 16:18   ` Darrick J. Wong
  2018-12-11 19:02   ` [PATCH] mkfs: fix symlink target if_bytes computation for protofile Darrick J. Wong
  1 sibling, 0 replies; 9+ messages in thread
From: Darrick J. Wong @ 2018-11-27 16:18 UTC (permalink / raw)
  To: Eric Sandeen; +Cc: Eric Sandeen, linux-xfs, Zorro Lang

On Mon, Nov 26, 2018 at 05:04:20PM -0600, Eric Sandeen wrote:
> 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 <sandeen@redhat.com>
> Reported-by: Zorro Lang <zlang@redhat.com>

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 <darrick.wong@oracle.com>

--D

> ---
> 
> V2: don't increase len, it gets set into i_size.  Just bump
> the allocation/copy by 1.  Thanks, Darrick.
> 
> diff --git a/mkfs/proto.c b/mkfs/proto.c
> index 1cd5436..103795f 100644
> --- a/mkfs/proto.c
> +++ b/mkfs/proto.c
> @@ -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;
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH] mkfs: fix symlink target if_bytes computation for protofile
  2018-11-26 23:04 ` [PATCH V2] " Eric Sandeen
  2018-11-27 16:18   ` Darrick J. Wong
@ 2018-12-11 19:02   ` Darrick J. Wong
  2018-12-11 20:36     ` Eric Sandeen
  1 sibling, 1 reply; 9+ messages in thread
From: Darrick J. Wong @ 2018-12-11 19:02 UTC (permalink / raw)
  To: Eric Sandeen; +Cc: Eric Sandeen, linux-xfs, Zorro Lang

From: Darrick J. Wong <darrick.wong@oracle.com>

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 <darrick.wong@oracle.com>
---
 mkfs/proto.c |   10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/mkfs/proto.c b/mkfs/proto.c
index fc07de5f..dc0225bd 100644
--- a/mkfs/proto.c
+++ b/mkfs/proto.c
@@ -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;

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH] mkfs: fix symlink target if_bytes computation for protofile
  2018-12-11 19:02   ` [PATCH] mkfs: fix symlink target if_bytes computation for protofile Darrick J. Wong
@ 2018-12-11 20:36     ` Eric Sandeen
  2018-12-11 22:42       ` Darrick J. Wong
  0 siblings, 1 reply; 9+ messages in thread
From: Eric Sandeen @ 2018-12-11 20:36 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: Eric Sandeen, linux-xfs, Zorro Lang

On 12/11/18 1:02 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> 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 <darrick.wong@oracle.com>

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 <sandeen@redhat.com>
---

diff --git a/libxfs/libxfs_api_defs.h b/libxfs/libxfs_api_defs.h
index ad5d65de..c3792e12 100644
--- a/libxfs/libxfs_api_defs.h
+++ b/libxfs/libxfs_api_defs.h
@@ -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__ */
diff --git a/mkfs/proto.c b/mkfs/proto.c
index 103795f1..3bba4917 100644
--- a/mkfs/proto.c
+++ b/mkfs/proto.c
@@ -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) {

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH] mkfs: fix symlink target if_bytes computation for protofile
  2018-12-11 20:36     ` Eric Sandeen
@ 2018-12-11 22:42       ` Darrick J. Wong
  2018-12-12  4:50         ` Darrick J. Wong
  0 siblings, 1 reply; 9+ messages in thread
From: Darrick J. Wong @ 2018-12-11 22:42 UTC (permalink / raw)
  To: Eric Sandeen; +Cc: Eric Sandeen, linux-xfs, Zorro Lang

On Tue, Dec 11, 2018 at 02:36:32PM -0600, Eric Sandeen wrote:
> On 12/11/18 1:02 PM, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > 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 <darrick.wong@oracle.com>
> 
> 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?

> 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

> =========
> 
> mkfs: don't open code local fork setup in protofile code
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> diff --git a/libxfs/libxfs_api_defs.h b/libxfs/libxfs_api_defs.h
> index ad5d65de..c3792e12 100644
> --- a/libxfs/libxfs_api_defs.h
> +++ b/libxfs/libxfs_api_defs.h
> @@ -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__ */
> diff --git a/mkfs/proto.c b/mkfs/proto.c
> index 103795f1..3bba4917 100644
> --- a/mkfs/proto.c
> +++ b/mkfs/proto.c
> @@ -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) {
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] mkfs: fix symlink target if_bytes computation for protofile
  2018-12-11 22:42       ` Darrick J. Wong
@ 2018-12-12  4:50         ` Darrick J. Wong
  0 siblings, 0 replies; 9+ messages in thread
From: Darrick J. Wong @ 2018-12-12  4:50 UTC (permalink / raw)
  To: Eric Sandeen; +Cc: Eric Sandeen, linux-xfs, Zorro Lang

On Tue, Dec 11, 2018 at 02:42:32PM -0800, Darrick J. Wong wrote:
> On Tue, Dec 11, 2018 at 02:36:32PM -0600, Eric Sandeen wrote:
> > On 12/11/18 1:02 PM, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > > 
> > > 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 <darrick.wong@oracle.com>
> > 
> > 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?
> 
> > 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.

Seems fine to me, so...
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> 
> --D
> 
> > =========
> > 
> > mkfs: don't open code local fork setup in protofile code
> > 
> > Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> > ---
> > 
> > diff --git a/libxfs/libxfs_api_defs.h b/libxfs/libxfs_api_defs.h
> > index ad5d65de..c3792e12 100644
> > --- a/libxfs/libxfs_api_defs.h
> > +++ b/libxfs/libxfs_api_defs.h
> > @@ -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__ */
> > diff --git a/mkfs/proto.c b/mkfs/proto.c
> > index 103795f1..3bba4917 100644
> > --- a/mkfs/proto.c
> > +++ b/mkfs/proto.c
> > @@ -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) {
> > 

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2018-12-12  4:50 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-26 22:39 [PATCH] mkfs.xfs: null-terminate symlinks created via protofile Eric Sandeen
2018-11-26 22:55 ` Darrick J. Wong
2018-11-26 23:01   ` Eric Sandeen
2018-11-26 23:04 ` [PATCH V2] " Eric Sandeen
2018-11-27 16:18   ` Darrick J. Wong
2018-12-11 19:02   ` [PATCH] mkfs: fix symlink target if_bytes computation for protofile Darrick J. Wong
2018-12-11 20:36     ` Eric Sandeen
2018-12-11 22:42       ` Darrick J. Wong
2018-12-12  4:50         ` Darrick J. Wong

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.