| Message ID | 20181211190256.GV24487@magnolia (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mkfs: fix symlink target if_bytes computation for protofile | expand |
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) {
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) { >
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) { > >
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;