Re: git: bfc8e3308bee - main - ext2fs: Fix the size of struct ufid and add a static assert

On Thu, Dec 5, 2024 at 7:15 PM Rick Macklem <rick.macklem@gmail.com> wrote:
>
> On Thu, Dec 5, 2024 at 6:40 PM Jessica Clarke <jrtc27@freebsd.org> wrote:
> >
> > CAUTION: This email originated from outside of the University of Guelph. Do not click links or open attachments unless you recognize the sender and know the content is safe. If in doubt, forward suspicious emails to IThelp@uoguelph.ca.
> >
> >
> > On 6 Dec 2024, at 02:06, Rick Macklem <rmacklem@FreeBSD.org> wrote:
> > >
> > > The branch main has been updated by rmacklem:
> > >
> > > URL: https://cgit.FreeBSD.org/src/commit/?id=bfc8e3308bee23d0f7836d57f32ed8d47da02627
> > >
> > > commit bfc8e3308bee23d0f7836d57f32ed8d47da02627
> > > Author:     Rick Macklem <rmacklem@FreeBSD.org>
> > > AuthorDate: 2024-12-06 02:05:06 +0000
> > > Commit:     Rick Macklem <rmacklem@FreeBSD.org>
> > > CommitDate: 2024-12-06 02:05:06 +0000
> > >
> > >    ext2fs: Fix the size of struct ufid and add a static assert
> > >
> > >    File system specific *fid structures are copied into the generic
> > >    struct fid defined in sys/mount.h.
> > >    As such, they cannot be larger than struct fid.
> > >
> > >    This patch packed the structure and checks via a __Static_assert().
> > >
> > >    Reviewed by:    markj
> > >    MFC after:      2 weeks
> > > ---
> > > sys/fs/ext2fs/ext2_vnops.c | 2 ++
> > > sys/fs/ext2fs/inode.h      | 2 +-
> > > 2 files changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/sys/fs/ext2fs/ext2_vnops.c b/sys/fs/ext2fs/ext2_vnops.c
> > > index dfbb11f75421..064c10bd18b2 100644
> > > --- a/sys/fs/ext2fs/ext2_vnops.c
> > > +++ b/sys/fs/ext2fs/ext2_vnops.c
> > > @@ -1889,6 +1889,8 @@ ext2_vptofh(struct vop_vptofh_args *ap)
> > > {
> > > struct inode *ip;
> > > struct ufid *ufhp;
> > > + _Static_assert(sizeof(struct ufid) <= sizeof(struct fid),
> > > +    "struct ufid cannot be larger than struct fid");
> > >
> > > ip = VTOI(ap->a_vp);
> > > ufhp = (struct ufid *)ap->a_fhp;
> > > diff --git a/sys/fs/ext2fs/inode.h b/sys/fs/ext2fs/inode.h
> > > index 9ee1b5672da6..63102deb10b0 100644
> > > --- a/sys/fs/ext2fs/inode.h
> > > +++ b/sys/fs/ext2fs/inode.h
> > > @@ -191,7 +191,7 @@ struct ufid {
> > > uint16_t ufid_pad; /* Force 32-bit alignment. */
> > > ino_t ufid_ino; /* File number (ino). */
> > > uint32_t ufid_gen; /* Generation number. */
> > > -};
> > > +} __packed;
> >
> > Why not just swap ufid_ino and ufid_gen? This will give worse codegen
> > on architectures that don’t assume fast unaligned access.
> You can commit such a change if you'd like, as far as I am concerned.
>
> I suspect few (if any) export ext2fs file systems anyhow, so this structure
> is unlikely to ever be used.
>
> There was a discussion on phabricator in D47879 about reordering
> entries and the consensus there seemed to be avoid doing that,
> although admittedly I think they were referring to the first two fields,
> which match the ones in the generic "struct fid".
>
> rick
ps: I used __packed since that was what markj@ used for cd9660,
      to keep things consistent.

>
> >
> > Jess
> >