From: Abhijith Das <adas@redhat.com> To: Jonathan Corbet <corbet@lwn.net> Cc: linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org, cluster-devel@redhat.com Subject: Re: [RFC PATCH 5/5] gfs2: Add xreaddir file operation and supporting functions Date: Tue, 29 Jul 2014 18:25:57 -0400 (EDT) [thread overview] Message-ID: <584806381.15003446.1406672757956.JavaMail.zimbra@redhat.com> (raw) In-Reply-To: <20140729145808.79bec165@lwn.net> ----- Original Message ----- > From: "Jonathan Corbet" <corbet@lwn.net> > To: "Abhi Das" <adas@redhat.com> > Cc: linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org, cluster-devel@redhat.com > Sent: Tuesday, July 29, 2014 1:58:08 PM > Subject: Re: [RFC PATCH 5/5] gfs2: Add xreaddir file operation and supporting functions > > On Fri, 25 Jul 2014 12:38:08 -0500 > Abhi Das <adas@redhat.com> wrote: > > > This patch adds support in GFS2 for the xgetdents syscall by > > implementing the xreaddir file operation. > > So I was trying to make sense of this, and ran into one little thing that > jumped out at me: > > > +static int gfs2_xrdir_to_user_vars(struct gfs2_xrdir_ctx *xc, > > + struct gfs2_xdirent *x, > > + struct gfs2_xdirent *x_vb_p, > > + struct linux_xdirent __user *lxd, > > + size_t count, size_t *bytes) > > Now, I'll readily admit that I could be overly confused by this function. > When the variables are named "x", "xx", "xc", "x_vb_p", "xblob", and "lxd", > it all starts to run together. But still... > As I went along writing this patch, I ended up needing more structs and names and this is the ugly result :(. I guess some of these structs/names can be reviewed and changed as needed. This is just a proof of concept patch to illustrate how such a syscall can be implemented. > > + if ((xc->xc_xattr_mask & XSTAT_XATTR_ALL) && > > + lxd->xd_blob.xb_xattr_count) { > > How can that be right? lxd is __user, it doesn't seem right to be > dereferencing it directly...? Wouldn't the call to access_ok() at the start of the syscall take care of this? All the __user pointers point to areas within the user supplied buffer buf and overflow past the end of the buffer for the last lxd is checked for. The 2/5 patch in this series adds the following in fs/readdir.c: +SYSCALL_DEFINE5(xgetdents, unsigned int, fd, unsigned, flags, unsigned int, mask, + void __user *, buf, unsigned int, count) ... ... ... + if (!access_ok(VERIFY_WRITE, buf, count)) + return -EFAULT; > > Thanks, > > jon > Cheers! --Abhi
WARNING: multiple messages have this Message-ID (diff)
From: Abhijith Das <adas@redhat.com> To: cluster-devel.redhat.com Subject: [Cluster-devel] [RFC PATCH 5/5] gfs2: Add xreaddir file operation and supporting functions Date: Tue, 29 Jul 2014 18:25:57 -0400 (EDT) [thread overview] Message-ID: <584806381.15003446.1406672757956.JavaMail.zimbra@redhat.com> (raw) In-Reply-To: <20140729145808.79bec165@lwn.net> ----- Original Message ----- > From: "Jonathan Corbet" <corbet@lwn.net> > To: "Abhi Das" <adas@redhat.com> > Cc: linux-kernel at vger.kernel.org, linux-fsdevel at vger.kernel.org, cluster-devel at redhat.com > Sent: Tuesday, July 29, 2014 1:58:08 PM > Subject: Re: [RFC PATCH 5/5] gfs2: Add xreaddir file operation and supporting functions > > On Fri, 25 Jul 2014 12:38:08 -0500 > Abhi Das <adas@redhat.com> wrote: > > > This patch adds support in GFS2 for the xgetdents syscall by > > implementing the xreaddir file operation. > > So I was trying to make sense of this, and ran into one little thing that > jumped out at me: > > > +static int gfs2_xrdir_to_user_vars(struct gfs2_xrdir_ctx *xc, > > + struct gfs2_xdirent *x, > > + struct gfs2_xdirent *x_vb_p, > > + struct linux_xdirent __user *lxd, > > + size_t count, size_t *bytes) > > Now, I'll readily admit that I could be overly confused by this function. > When the variables are named "x", "xx", "xc", "x_vb_p", "xblob", and "lxd", > it all starts to run together. But still... > As I went along writing this patch, I ended up needing more structs and names and this is the ugly result :(. I guess some of these structs/names can be reviewed and changed as needed. This is just a proof of concept patch to illustrate how such a syscall can be implemented. > > + if ((xc->xc_xattr_mask & XSTAT_XATTR_ALL) && > > + lxd->xd_blob.xb_xattr_count) { > > How can that be right? lxd is __user, it doesn't seem right to be > dereferencing it directly...? Wouldn't the call to access_ok() at the start of the syscall take care of this? All the __user pointers point to areas within the user supplied buffer buf and overflow past the end of the buffer for the last lxd is checked for. The 2/5 patch in this series adds the following in fs/readdir.c: +SYSCALL_DEFINE5(xgetdents, unsigned int, fd, unsigned, flags, unsigned int, mask, + void __user *, buf, unsigned int, count) ... ... ... + if (!access_ok(VERIFY_WRITE, buf, count)) + return -EFAULT; > > Thanks, > > jon > Cheers! --Abhi
next prev parent reply other threads:[~2014-07-29 22:26 UTC|newest] Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top 2014-07-25 17:38 [RFC PATCH 0/5] xgetdents system call Abhi Das 2014-07-25 17:38 ` [Cluster-devel] " Abhi Das 2014-07-25 17:38 ` [RFC PATCH 1/5] fs: xstat system call VFS bits Abhi Das 2014-07-25 17:38 ` [Cluster-devel] " Abhi Das 2014-07-25 17:38 ` Abhi Das 2014-07-25 18:17 ` [Cluster-devel] " Bob Peterson 2014-07-25 18:17 ` Bob Peterson 2014-07-25 17:38 ` [RFC PATCH 2/5] fs: Add xgetdents system call and xreaddir file operation Abhi Das 2014-07-25 17:38 ` [Cluster-devel] " Abhi Das 2014-07-25 17:38 ` Abhi Das 2014-07-29 8:20 ` Michael Kerrisk 2014-07-29 8:20 ` [Cluster-devel] " Michael Kerrisk 2014-07-29 8:20 ` Michael Kerrisk 2014-07-25 17:38 ` [RFC PATCH 3/5] gfs2: Add a dynamic buffer backed by a vector of pages Abhi Das 2014-07-25 17:38 ` [Cluster-devel] " Abhi Das 2014-07-25 17:38 ` Abhi Das 2014-07-25 18:42 ` [Cluster-devel] " Bob Peterson 2014-07-25 18:42 ` Bob Peterson 2014-07-25 17:38 ` [RFC PATCH 4/5] gfs2: Add sort functionality with extra parameter Abhi Das 2014-07-25 17:38 ` [Cluster-devel] " Abhi Das 2014-07-25 17:38 ` [RFC PATCH 5/5] gfs2: Add xreaddir file operation and supporting functions Abhi Das 2014-07-25 17:38 ` [Cluster-devel] " Abhi Das 2014-07-29 18:58 ` Jonathan Corbet 2014-07-29 18:58 ` [Cluster-devel] " Jonathan Corbet 2014-07-29 22:25 ` Abhijith Das [this message] 2014-07-29 22:25 ` Abhijith Das 2014-07-30 9:06 ` Steven Whitehouse 2014-07-30 13:57 ` Jonathan Corbet 2014-07-30 13:57 ` [Cluster-devel] " Jonathan Corbet 2014-07-29 8:18 ` [RFC PATCH 0/5] xgetdents system call Michael Kerrisk 2014-07-29 8:18 ` [Cluster-devel] " Michael Kerrisk 2014-07-29 8:18 ` Michael Kerrisk
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=584806381.15003446.1406672757956.JavaMail.zimbra@redhat.com \ --to=adas@redhat.com \ --cc=cluster-devel@redhat.com \ --cc=corbet@lwn.net \ --cc=linux-fsdevel@vger.kernel.org \ --cc=linux-kernel@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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.