On Mon, Apr 15, 2019 at 4:41 PM Patrick Donnelly <pdonnell@xxxxxxxxxx> wrote:
>
> On Mon, Apr 15, 2019 at 9:45 AM Jeff Layton <jlayton@xxxxxxxxxx> wrote:
> >
> > Ben reported tripping the BUG_ON in create_request_message during some
> > performance testing. Analysis of the vmcore showed that the length of
> > the r_dentry->d_name string changed after we allocated the buffer, but
> > before we encoded it.
> >
> > build_dentry_path returns pointers to d_name in the common case of
> > non-snapped dentries, but this optimization isn't safe. Instead, make a
> > copy of the string in this case. That is less efficient, but safe.
> >
> > Reported-by: Ben England <bengland@xxxxxxxxxx>
> > Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> > ---
> > fs/ceph/mds_client.c | 40 ++++++++++++++++++++++++++++++++--------
> > 1 file changed, 32 insertions(+), 8 deletions(-)
> >
> > diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> > index e09feadd2ae1..4cfefe118128 100644
> > --- a/fs/ceph/mds_client.c
> > +++ b/fs/ceph/mds_client.c
> > @@ -2159,9 +2159,35 @@ char *ceph_mdsc_build_path(struct dentry *dentry, int *plen, u64 *base,
> > return path;
> > }
> >
> > +/* Duplicate the dentry->d_name.name safely */
> > +static int clone_dentry_name(struct dentry *dentry, const char **ppath,
> > + int *ppathlen)
> > +{
> > + u32 len;
> > + char *name;
> > +retry:
> > + len = READ_ONCE(dentry->d_name.len);
> > + name = kmalloc(len + 1, GFP_NOFS);
> > + if (!name)
> > + return -ENOMEM;
> > +
> > + spin_lock(&dentry->d_lock);
> > + if (dentry->d_name.len != len) {
> > + spin_unlock(&dentry->d_lock);
> > + kfree(name);
> > + goto retry;
> > + }
> > + memcpy(name, dentry->d_name.name, len);
> > + spin_unlock(&dentry->d_lock);
> > +
> > + name[len] = '\0';
> > + *ppath = name;
> > + *ppathlen = len;
> > + return 0;
> > +}
>
> I would think passing a char array of PATH_MAX would be preferable to
> a heap allocation. Is that not typical?
>
In general I don't like doing big allocations on the stack in the
kernel, as it is a finite resource.
That said, we'd technically only need a NAME_MAX+1 (256 byte) array
here, which is not _too_ bad. If that's what the maintainers prefer,
we could do that instead.
--
Jeff Layton <jlayton@xxxxxxxxxx>
