diff mbox

xfs: in _attrlist_by_handle, copy the cursor back to userspace

Message ID 20160802035651.GB8593@birch.djwong.org (mailing list archive)
State Accepted
Headers show

Commit Message

Darrick J. Wong Aug. 2, 2016, 3:56 a.m. UTC
When we're iterating inode xattrs by handle, we have to copy the
cursor back to userspace so that a subsequent invocation actually
retrieves subsequent contents.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/xfs_ioctl.c |    6 ++++++
 1 file changed, 6 insertions(+)

Comments

Christoph Hellwig Aug. 2, 2016, 12:25 p.m. UTC | #1
On Mon, Aug 01, 2016 at 08:56:51PM -0700, Darrick J. Wong wrote:
> When we're iterating inode xattrs by handle, we have to copy the
> cursor back to userspace so that a subsequent invocation actually
> retrieves subsequent contents.

Testcase?
Darrick J. Wong Aug. 2, 2016, 3:40 p.m. UTC | #2
On Tue, Aug 02, 2016 at 05:25:17AM -0700, Christoph Hellwig wrote:
> On Mon, Aug 01, 2016 at 08:56:51PM -0700, Darrick J. Wong wrote:
> > When we're iterating inode xattrs by handle, we have to copy the
> > cursor back to userspace so that a subsequent invocation actually
> > retrieves subsequent contents.
> 
> Testcase?

Found it while continuing development of xfs_scrub.  I'll send along the
xfstest patch when I've finished polishing it.  (It was harder than usual since
xfs_io doesn't /use/ the attr-by-handle interface... nothing does.)

--D
Dave Chinner Aug. 2, 2016, 9:41 p.m. UTC | #3
On Tue, Aug 02, 2016 at 08:40:35AM -0700, Darrick J. Wong wrote:
> On Tue, Aug 02, 2016 at 05:25:17AM -0700, Christoph Hellwig wrote:
> > On Mon, Aug 01, 2016 at 08:56:51PM -0700, Darrick J. Wong wrote:
> > > When we're iterating inode xattrs by handle, we have to copy the
> > > cursor back to userspace so that a subsequent invocation actually
> > > retrieves subsequent contents.
> > 
> > Testcase?
> 
> Found it while continuing development of xfs_scrub.  I'll send along the
> xfstest patch when I've finished polishing it.  (It was harder than usual since
> xfs_io doesn't /use/ the attr-by-handle interface... nothing does.)

It was (and probably still is) used by SGI's HSM. I thought there
was some coverage of the interface in the dmapi part of the xfstests
suite, but perhaps it's only tested by SGI's internal hsm test
suite...

Cheers,

Dave.
Mark Tinguely Aug. 2, 2016, 10:47 p.m. UTC | #4
On 08/02/16 16:41, Dave Chinner wrote:
> On Tue, Aug 02, 2016 at 08:40:35AM -0700, Darrick J. Wong wrote:
>> On Tue, Aug 02, 2016 at 05:25:17AM -0700, Christoph Hellwig wrote:
>>> On Mon, Aug 01, 2016 at 08:56:51PM -0700, Darrick J. Wong wrote:
>>>> When we're iterating inode xattrs by handle, we have to copy the
>>>> cursor back to userspace so that a subsequent invocation actually
>>>> retrieves subsequent contents.
>>> Testcase?
>> Found it while continuing development of xfs_scrub.  I'll send along the
>> xfstest patch when I've finished polishing it.  (It was harder than usual since
>> xfs_io doesn't /use/ the attr-by-handle interface... nothing does.)
> It was (and probably still is) used by SGI's HSM. I thought there
> was some coverage of the interface in the dmapi part of the xfstests
> suite, but perhaps it's only tested by SGI's internal hsm test
> suite...
>
> Cheers,
>
> Dave.

You may be thinking of the DMAPI specific extended attribute interface 
and that is tested in xfstests. That function cannot cursor.

This change would be appropriate to xfsdump (dumping extended attributes 
via jdm_attr_list()) than our HSM.

I don't see a SGI test for attr_list_by_handle() nor jdm_attr_list(). 
There is mention in the xfstests source file src/open_unlink.c but that 
does not cursor.

--Mark Tinguely.
Darrick J. Wong Aug. 2, 2016, 11:57 p.m. UTC | #5
On Tue, Aug 02, 2016 at 08:40:35AM -0700, Darrick J. Wong wrote:
> On Tue, Aug 02, 2016 at 05:25:17AM -0700, Christoph Hellwig wrote:
> > On Mon, Aug 01, 2016 at 08:56:51PM -0700, Darrick J. Wong wrote:
> > > When we're iterating inode xattrs by handle, we have to copy the
> > > cursor back to userspace so that a subsequent invocation actually
> > > retrieves subsequent contents.
> > 
> > Testcase?
> 
> Found it while continuing development of xfs_scrub.  I'll send along the
> xfstest patch when I've finished polishing it.  (It was harder than usual since
> xfs_io doesn't /use/ the attr-by-handle interface... nothing does.)

Found some extra time while I run all of today's rmap changes through QA
to push to Dave, so I fixed (I hope) the remaining problems I had and
sent out a testcase.

--D

> 
> --D
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
diff mbox

Patch

diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 2ef22db..30e69f5 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -390,6 +390,7 @@  xfs_attrlist_by_handle(
 {
 	int			error = -ENOMEM;
 	attrlist_cursor_kern_t	*cursor;
+	struct xfs_fsop_attrlist_handlereq __user	*p = arg;
 	xfs_fsop_attrlist_handlereq_t al_hreq;
 	struct dentry		*dentry;
 	char			*kbuf;
@@ -422,6 +423,11 @@  xfs_attrlist_by_handle(
 	if (error)
 		goto out_kfree;
 
+	if (copy_to_user(&p->pos, cursor, sizeof(attrlist_cursor_kern_t))) {
+		error = -EFAULT;
+		goto out_kfree;
+	}
+
 	if (copy_to_user(al_hreq.buffer, kbuf, al_hreq.buflen))
 		error = -EFAULT;