From: "Darrick J. Wong" <djwong@kernel.org>
To: Christoph Hellwig <hch@infradead.org>
Cc: Kees Cook <keescook@chromium.org>,
Carlos Maiolino <cem@kernel.org>,
Dave Chinner <david@fromorbit.com>,
Jeff Layton <jlayton@kernel.org>,
Eric Biggers <ebiggers@kernel.org>,
"Gustavo A. R. Silva" <gustavoars@kernel.org>,
linux-xfs@vger.kernel.org, linux-hardening@vger.kernel.org
Subject: Re: [PATCH] libxfs: Redefine 1-element arrays as flexible arrays
Date: Wed, 12 Jul 2023 22:44:50 -0700 [thread overview]
Message-ID: <20230713054450.GQ108251@frogsfrogsfrogs> (raw)
In-Reply-To: <ZK6mC1npjONMoGMD@infradead.org>
On Wed, Jul 12, 2023 at 06:09:31AM -0700, Christoph Hellwig wrote:
> On Tue, Jul 11, 2023 at 10:37:38PM -0700, Darrick J. Wong wrote:
> > Here's my version, where I go for a straight a[1] -> a[] conversion and
> > let downstream attrlist ioctl users eat their lumps. What do you think
> > of the excess-documentation approach?
>
> I think this is roughtly the right thing, with one big caveat:
>
> > + /* In Linux 6.5 this flex array was changed from list[1] to list[]. */
>
> For all the format headers there's no need for these comments. We've
> done this for various other structures that had the old one size arrays
> and never bothered.
<nod> Dave looked at an earlier version and wanted the comments for
xfs_da_format.h to steer people at the entsize helpers. I more or less
agree that it's overkill everywhere else though.
> > diff --git a/libxfs/xfs_fs.h b/libxfs/xfs_fs.h
> > index 9c60ebb3..8927ecb2 100644
> > --- a/libxfs/xfs_fs.h
> > +++ b/libxfs/xfs_fs.h
> > @@ -588,16 +588,19 @@ typedef struct xfs_attrlist_cursor {
> > *
> > * NOTE: struct xfs_attrlist must match struct attrlist defined in libattr, and
> > * struct xfs_attrlist_ent must match struct attrlist_ent defined in libattr.
> > + *
> > + * WARNING: In Linux 6.5, al_offset and a_name were changed from array[1] to
> > + * array[]. Anyone using sizeof is (already) broken!
> > */
> > struct xfs_attrlist {
> > __s32 al_count; /* number of entries in attrlist */
> > __s32 al_more; /* T/F: more attrs (do call again) */
> > - __s32 al_offset[1]; /* byte offsets of attrs [var-sized] */
> > + __s32 al_offset[]; /* byte offsets of attrs [var-sized] */
> > };
> >
> > struct xfs_attrlist_ent { /* data from attr_list() */
> > __u32 a_valuelen; /* number bytes in value of attr */
> > - char a_name[1]; /* attr name (NULL terminated) */
> > + char a_name[]; /* attr name (NULL terminated) */
> > };
>
> Now these are currently exposed in a xfsprogs headeer and IFF someone
> is using them it would break them in nasty ways. That being said,
> xfsprogs itself doesn't use them as it relies on identically layed
> out but differntly named structures from libattr.
>
> So I think we should just move these two out of xfs_fs.h, because
> while they document a UAPI, they aren't actually used by userspace.
>
> With that the need for any caution goes away and we can just fix the
> definition without any caveats.
So I looked at the libattr headers for Ubuntu 22.04 and saw this:
/*
* List the names and sizes of the values of all the attributes of an object.
* "Cursor" must be allocated and zeroed before the first call, it is used
* to maintain context between system calls if all the attribute names won't
* fit into the buffer on the first system call.
* The return value is -1 on error (w/errno set appropriately), 0 on success.
*/
extern int attr_list(const char *__path, char *__buffer, const int __buffersize,
int __flags, attrlist_cursor_t *__cursor)
__attribute__ ((deprecated ("Use listxattr or llistxattr instead")));
extern int attr_listf(int __fd, char *__buffer, const int __buffersize,
int __flags, attrlist_cursor_t *__cursor)
__attribute__ ((deprecated ("Use flistxattr instead")));
I take that as a sign that they could drop all these xfs-specific APIs
one day, which means we ought to keep them in xfs_fs.h.
--D
next prev parent reply other threads:[~2023-07-13 5:44 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-07-11 22:20 [PATCH] libxfs: Redefine 1-element arrays as flexible arrays Kees Cook
2023-07-12 5:37 ` Darrick J. Wong
2023-07-12 13:09 ` Christoph Hellwig
2023-07-13 5:44 ` Darrick J. Wong [this message]
2023-07-13 5:54 ` Christoph Hellwig
2023-07-12 18:45 ` Kees Cook
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20230713054450.GQ108251@frogsfrogsfrogs \
--to=djwong@kernel.org \
--cc=cem@kernel.org \
--cc=david@fromorbit.com \
--cc=ebiggers@kernel.org \
--cc=gustavoars@kernel.org \
--cc=hch@infradead.org \
--cc=jlayton@kernel.org \
--cc=keescook@chromium.org \
--cc=linux-hardening@vger.kernel.org \
--cc=linux-xfs@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).