| Message ID | 20200217125957.263434-24-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/31] xfs: reject invalid flags combinations in XFS_IOC_ATTRLIST_BY_HANDLE | expand |
On Mon, Feb 17, 2020 at 01:59:49PM +0100, Christoph Hellwig wrote: > The buffer field always points to a strut xfs_attrlist, so use the > proper type. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > fs/xfs/libxfs/xfs_fs.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/xfs/libxfs/xfs_fs.h b/fs/xfs/libxfs/xfs_fs.h > index ae77bcd8c05b..21920f613d42 100644 > --- a/fs/xfs/libxfs/xfs_fs.h > +++ b/fs/xfs/libxfs/xfs_fs.h > @@ -597,7 +597,7 @@ typedef struct xfs_fsop_attrlist_handlereq { > struct xfs_attrlist_cursor pos; /* opaque cookie, list offset */ > __u32 flags; /* which namespace to use */ > __u32 buflen; /* length of buffer supplied */ > - void __user *buffer; /* returned names */ > + struct xfs_attrlist __user *buffer;/* returned names */ > } xfs_fsop_attrlist_handlereq_t; This changes the userspace API, right? So, in theory, it could break compilation of userspace applications that treat it as an attrlist_t and don't specifically cast the assignment because it's currently a void pointer? Cheers, Dave.
On Tue, Feb 18, 2020 at 10:53:16AM +1100, Dave Chinner wrote: > > diff --git a/fs/xfs/libxfs/xfs_fs.h b/fs/xfs/libxfs/xfs_fs.h > > index ae77bcd8c05b..21920f613d42 100644 > > --- a/fs/xfs/libxfs/xfs_fs.h > > +++ b/fs/xfs/libxfs/xfs_fs.h > > @@ -597,7 +597,7 @@ typedef struct xfs_fsop_attrlist_handlereq { > > struct xfs_attrlist_cursor pos; /* opaque cookie, list offset */ > > __u32 flags; /* which namespace to use */ > > __u32 buflen; /* length of buffer supplied */ > > - void __user *buffer; /* returned names */ > > + struct xfs_attrlist __user *buffer;/* returned names */ > > } xfs_fsop_attrlist_handlereq_t; > > This changes the userspace API, right? So, in theory, it could break > compilation of userspace applications that treat it as an attrlist_t > and don't specifically cast the assignment because it's currently > a void pointer? IFF userspace was using this header it would change the API. But userspace uses the libattr definition exclusively.
On Tue, Feb 18, 2020 at 04:39:24PM +0100, Christoph Hellwig wrote: > On Tue, Feb 18, 2020 at 10:53:16AM +1100, Dave Chinner wrote: > > > diff --git a/fs/xfs/libxfs/xfs_fs.h b/fs/xfs/libxfs/xfs_fs.h > > > index ae77bcd8c05b..21920f613d42 100644 > > > --- a/fs/xfs/libxfs/xfs_fs.h > > > +++ b/fs/xfs/libxfs/xfs_fs.h > > > @@ -597,7 +597,7 @@ typedef struct xfs_fsop_attrlist_handlereq { > > > struct xfs_attrlist_cursor pos; /* opaque cookie, list offset */ > > > __u32 flags; /* which namespace to use */ > > > __u32 buflen; /* length of buffer supplied */ > > > - void __user *buffer; /* returned names */ > > > + struct xfs_attrlist __user *buffer;/* returned names */ > > > } xfs_fsop_attrlist_handlereq_t; > > > > This changes the userspace API, right? So, in theory, it could break > > compilation of userspace applications that treat it as an attrlist_t > > and don't specifically cast the assignment because it's currently > > a void pointer? > > IFF userspace was using this header it would change the API. But > userspace uses the libattr definition exclusively. Assuming most userspace will use libhandle (and not call the ioctl directly) then this "shouldn't" be a problem because libhandle treats the attrlist buffer as a void pointer. (I dunno, how difficult /is/ it to say "program to the library, not the kernel ABI" here?) --D
On Tue, Feb 18, 2020 at 04:58:47PM -0800, Darrick J. Wong wrote: > On Tue, Feb 18, 2020 at 04:39:24PM +0100, Christoph Hellwig wrote: > > On Tue, Feb 18, 2020 at 10:53:16AM +1100, Dave Chinner wrote: > > > > diff --git a/fs/xfs/libxfs/xfs_fs.h b/fs/xfs/libxfs/xfs_fs.h > > > > index ae77bcd8c05b..21920f613d42 100644 > > > > --- a/fs/xfs/libxfs/xfs_fs.h > > > > +++ b/fs/xfs/libxfs/xfs_fs.h > > > > @@ -597,7 +597,7 @@ typedef struct xfs_fsop_attrlist_handlereq { > > > > struct xfs_attrlist_cursor pos; /* opaque cookie, list offset */ > > > > __u32 flags; /* which namespace to use */ > > > > __u32 buflen; /* length of buffer supplied */ > > > > - void __user *buffer; /* returned names */ > > > > + struct xfs_attrlist __user *buffer;/* returned names */ > > > > } xfs_fsop_attrlist_handlereq_t; > > > > > > This changes the userspace API, right? So, in theory, it could break > > > compilation of userspace applications that treat it as an attrlist_t > > > and don't specifically cast the assignment because it's currently > > > a void pointer? > > > > IFF userspace was using this header it would change the API. But > > userspace uses the libattr definition exclusively. > > Assuming most userspace will use libhandle (and not call the ioctl > directly) then this "shouldn't" be a problem because libhandle treats > the attrlist buffer as a void pointer. There's a lot of "if's" there. :/ All I'm asking for is that the changes are documented as known and intentional so that we don't end up a couple of years down the track wondering WTF we were thinking when we made this change... > (I dunno, how difficult /is/ it to say "program to the library, not the > kernel ABI" here?) The xfsctl(3) man page already says this: XFS_IOC_PATH_TO_HANDLE XFS_IOC_PATH_TO_FSHANDLE XFS_IOC_FD_TO_HANDLE XFS_IOC_OPEN_BY_HANDLE XFS_IOC_READLINK_BY_HANDLE XFS_IOC_ATTR_LIST_BY_HANDLE XFS_IOC_ATTR_MULTI_BY_HANDLE XFS_IOC_FSSETDM_BY_HANDLE These are all interfaces that are used to implement various libhandle functions (see open_by_handle(3)). They are all subject to change and should not be called directly by applications. and the open_by_handle(3) man page says "use libhandle to access these functions". Cheers, Dave.
diff --git a/fs/xfs/libxfs/xfs_fs.h b/fs/xfs/libxfs/xfs_fs.h index ae77bcd8c05b..21920f613d42 100644 --- a/fs/xfs/libxfs/xfs_fs.h +++ b/fs/xfs/libxfs/xfs_fs.h @@ -597,7 +597,7 @@ typedef struct xfs_fsop_attrlist_handlereq { struct xfs_attrlist_cursor pos; /* opaque cookie, list offset */ __u32 flags; /* which namespace to use */ __u32 buflen; /* length of buffer supplied */ - void __user *buffer; /* returned names */ + struct xfs_attrlist __user *buffer;/* returned names */ } xfs_fsop_attrlist_handlereq_t; typedef struct xfs_attr_multiop {
The buffer field always points to a strut xfs_attrlist, so use the proper type. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/xfs/libxfs/xfs_fs.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)