Re: [RFC PATCH 8/9] debugfs: defer debugfs_fsdata allocation to first usage

[Johannes Berg]

On Tue, 2017-05-02 at 22:05 +0200, Nicolai Stange wrote:

> > So either you could return some valid ops (perhaps
> > debugfs_noop_file_operations although those don't have .name or
> > .poll, so it doesn't cover everything), or you can just BUG_ON()
> > here directly, saving the incomprehensible crash later.
> 
> The purpose of that WARN_ON() there was to turn a potentially
> incomprehensible crash into a comprehensible one ;)
> 
> In order to avoid a new BUG_ON(), what about keeping the WARN_ON() as
> is and returning NULL instead of the garbage? That would crash
> current on first access and the earlier warning would hopefully give
> some clue?

Yeah, I guess that might work. Probably less harmful to the system than
a BUG_ON(), but I still operate under the assumption that there might
actually be something mapped at NULL - not sure if that's still true.

johannes

Re: [RFC PATCH 8/9] debugfs: defer debugfs_fsdata allocation to first usage

[Johannes Berg]

On Tue, 2017-05-02 at 22:05 +0200, Nicolai Stange wrote:

> > So either you could return some valid ops (perhaps
> > debugfs_noop_file_operations although those don't have .name or
> > .poll, so it doesn't cover everything), or you can just BUG_ON()
> > here directly, saving the incomprehensible crash later.
> 
> The purpose of that WARN_ON() there was to turn a potentially
> incomprehensible crash into a comprehensible one ;)
> 
> In order to avoid a new BUG_ON(), what about keeping the WARN_ON() as
> is and returning NULL instead of the garbage? That would crash
> current on first access and the earlier warning would hopefully give
> some clue?

Yeah, I guess that might work. Probably less harmful to the system than
a BUG_ON(), but I still operate under the assumption that there might
actually be something mapped at NULL - not sure if that's still true.

johannes

Re: [RFC PATCH 8/9] debugfs: defer debugfs_fsdata allocation to first usage

[Nicolai Stange]

On Tue, Apr 18 2017, Johannes Berg wrote:

> On Sun, 2017-04-16 at 11:51 +0200, Nicolai Stange wrote:
>> 
>> +++ b/fs/debugfs/file.c
>> @@ -53,6 +53,7 @@ const struct file_operations
>> *debugfs_real_fops(const struct file *filp)
>>  {
>>  struct debugfs_fsdata *fsd = F_DENTRY(filp)->d_fsdata;
>>  
>> +WARN_ON((unsigned long)fsd &
>> DEBUGFS_FSDATA_IS_REAL_FOPS_BIT);
>>  return fsd->real_fops;
>
> I'm not a fan of BUG_ON(), but in this case, if you have a completely
> bogus pointer here, and then you return fsd->real_fops which will be
> even more bogus, and *then* you call a function from within it... that
> seems like a recipe for disaster.

Hmm. Note that debugfs_real_fops() is provided for users outside of the
debugfs core which encapsulate their file_operations in a larger struct
and access this one through a container_of(debugfs_real_fops(dentry), ...).
Now, what the above WARN_ON() really checks for is whether the
debugfs_real_fops() has been called under a debugfs_file_get()/-_put()
pair. The point of requiring this isn't protection ([1]) or anything, but
merely to be able to streamline the implementation of
debugfs_real_fops(): knowing that a debugfs_file_get() is active allows
for the omission of the WARNed_ON case, namely that the fops are
encoded in ->d_fsdata itself.


> So either you could return some valid ops (perhaps
> debugfs_noop_file_operations although those don't have .name or .poll,
> so it doesn't cover everything), or you can just BUG_ON() here
> directly, saving the incomprehensible crash later.

The purpose of that WARN_ON() there was to turn a potentially
incomprehensible crash into a comprehensible one ;)

In order to avoid a new BUG_ON(), what about keeping the WARN_ON() as is
and returning NULL instead of the garbage? That would crash current on
first access and the earlier warning would hopefully give some clue?

Thanks,

Nicolai


[1] "protection" in the sense of "protection against file removal".  With
the proposed [9/9] ("debugfs: free debugfs_fsdata instances"), the
patch introducing occasional freeing of ->d_fsdata, an additional
RCU read side section would be needed if debugfs_real_fops() were
allowed to be called outside of a debugfs_file_get()/-_put() pair.


>>  EXPORT_SYMBOL_GPL(debugfs_real_fops);
>> @@ -74,9 +75,35 @@ EXPORT_SYMBOL_GPL(debugfs_real_fops);
>>   */
>>  int debugfs_file_get(struct dentry *dentry)
>>  {
>> -struct debugfs_fsdata *fsd = dentry->d_fsdata;
>> +struct debugfs_fsdata *fsd;
>> +void *d_fsd;
>> +
>> +d_fsd = READ_ONCE(dentry->d_fsdata);
>> +if (!((unsigned long)d_fsd &
>> DEBUGFS_FSDATA_IS_REAL_FOPS_BIT)) {
>> +fsd = d_fsd;
>> +} else {
>> +fsd = kmalloc(sizeof(*fsd), GFP_KERNEL);
>> +if (!fsd)
>> +return -ENOMEM;
>> +
>> +fsd->real_fops = (void *)((unsigned long)d_fsd &
>> +~DEBUGFS_FSDATA_IS_REAL_FOPS
>> _BIT);
>> +refcount_set(>active_users, 1);
>> +init_completion(>active_users_drained);
>> +if (cmpxchg(>d_fsdata, d_fsd, fsd) != d_fsd)
>> {
>> +kfree(fsd);
>> +fsd = READ_ONCE(dentry->d_fsdata);
>> +}
>> +}
>>  
>> -/* Avoid starvation of removers. */
>> +/*
>> + * In case of a successful cmpxchg() above, this check is
>> + * strictly necessary and must follow it, see the comment in
>> + * __debugfs_remove_file().
>> + * OTOH, if the cmpxchg() hasn't been executed or wasn't
>> + * successful, this serves the purpose of not starving
>> + * removers.
>> + */
>>  if (d_unlinked(dentry))
>>  return -EIO;
>>  
>> @@ -98,7 +125,7 @@ EXPORT_SYMBOL_GPL(debugfs_file_get);
>>   */
>>  void debugfs_file_put(struct dentry *dentry)
>>  {
>> -struct debugfs_fsdata *fsd = dentry->d_fsdata;
>> +struct debugfs_fsdata *fsd = READ_ONCE(dentry->d_fsdata);
>>  
>>  if (refcount_dec_and_test(>active_users))
>>  complete(>active_users_drained);
>> @@ -109,10 +136,11 @@ static int open_proxy_open(struct inode *inode,
>> struct file *filp)
>>  {
>>  struct dentry *dentry = F_DENTRY(filp);
>>  const struct file_operations *real_fops = NULL;
>> -int r = 0;
>> +int r;
>>  
>> -if (debugfs_file_get(dentry))
>> -return -ENOENT;
>> +r = debugfs_file_get(dentry);
>> +if (r)
>> +return r == -EIO ? -ENOENT : r;
>>  
>>  real_fops = debugfs_real_fops(filp);
>>  real_fops = fops_get(real_fops);
>> @@ -233,10 +261,11 @@ static int full_proxy_open(struct inode *inode,
>> struct file *filp)
>>  struct dentry *dentry = F_DENTRY(filp);
>>  const struct file_operations *real_fops = NULL;
>>  struct file_operations *proxy_fops = NULL;
>> -int r = 0;
>> +int r;
>>  
>> -if (debugfs_file_get(dentry))
>> -return -ENOENT;
>> +r = debugfs_file_get(dentry);
>> +if 

Re: [RFC PATCH 8/9] debugfs: defer debugfs_fsdata allocation to first usage

[Nicolai Stange]

On Tue, Apr 18 2017, Johannes Berg wrote:

> On Sun, 2017-04-16 at 11:51 +0200, Nicolai Stange wrote:
>> 
>> +++ b/fs/debugfs/file.c
>> @@ -53,6 +53,7 @@ const struct file_operations
>> *debugfs_real_fops(const struct file *filp)
>>  {
>>  struct debugfs_fsdata *fsd = F_DENTRY(filp)->d_fsdata;
>>  
>> +WARN_ON((unsigned long)fsd &
>> DEBUGFS_FSDATA_IS_REAL_FOPS_BIT);
>>  return fsd->real_fops;
>
> I'm not a fan of BUG_ON(), but in this case, if you have a completely
> bogus pointer here, and then you return fsd->real_fops which will be
> even more bogus, and *then* you call a function from within it... that
> seems like a recipe for disaster.

Hmm. Note that debugfs_real_fops() is provided for users outside of the
debugfs core which encapsulate their file_operations in a larger struct
and access this one through a container_of(debugfs_real_fops(dentry), ...).
Now, what the above WARN_ON() really checks for is whether the
debugfs_real_fops() has been called under a debugfs_file_get()/-_put()
pair. The point of requiring this isn't protection ([1]) or anything, but
merely to be able to streamline the implementation of
debugfs_real_fops(): knowing that a debugfs_file_get() is active allows
for the omission of the WARNed_ON case, namely that the fops are
encoded in ->d_fsdata itself.


> So either you could return some valid ops (perhaps
> debugfs_noop_file_operations although those don't have .name or .poll,
> so it doesn't cover everything), or you can just BUG_ON() here
> directly, saving the incomprehensible crash later.

The purpose of that WARN_ON() there was to turn a potentially
incomprehensible crash into a comprehensible one ;)

In order to avoid a new BUG_ON(), what about keeping the WARN_ON() as is
and returning NULL instead of the garbage? That would crash current on
first access and the earlier warning would hopefully give some clue?

Thanks,

Nicolai


[1] "protection" in the sense of "protection against file removal".  With
the proposed [9/9] ("debugfs: free debugfs_fsdata instances"), the
patch introducing occasional freeing of ->d_fsdata, an additional
RCU read side section would be needed if debugfs_real_fops() were
allowed to be called outside of a debugfs_file_get()/-_put() pair.


>>  EXPORT_SYMBOL_GPL(debugfs_real_fops);
>> @@ -74,9 +75,35 @@ EXPORT_SYMBOL_GPL(debugfs_real_fops);
>>   */
>>  int debugfs_file_get(struct dentry *dentry)
>>  {
>> -struct debugfs_fsdata *fsd = dentry->d_fsdata;
>> +struct debugfs_fsdata *fsd;
>> +void *d_fsd;
>> +
>> +d_fsd = READ_ONCE(dentry->d_fsdata);
>> +if (!((unsigned long)d_fsd &
>> DEBUGFS_FSDATA_IS_REAL_FOPS_BIT)) {
>> +fsd = d_fsd;
>> +} else {
>> +fsd = kmalloc(sizeof(*fsd), GFP_KERNEL);
>> +if (!fsd)
>> +return -ENOMEM;
>> +
>> +fsd->real_fops = (void *)((unsigned long)d_fsd &
>> +~DEBUGFS_FSDATA_IS_REAL_FOPS
>> _BIT);
>> +refcount_set(>active_users, 1);
>> +init_completion(>active_users_drained);
>> +if (cmpxchg(>d_fsdata, d_fsd, fsd) != d_fsd)
>> {
>> +kfree(fsd);
>> +fsd = READ_ONCE(dentry->d_fsdata);
>> +}
>> +}
>>  
>> -/* Avoid starvation of removers. */
>> +/*
>> + * In case of a successful cmpxchg() above, this check is
>> + * strictly necessary and must follow it, see the comment in
>> + * __debugfs_remove_file().
>> + * OTOH, if the cmpxchg() hasn't been executed or wasn't
>> + * successful, this serves the purpose of not starving
>> + * removers.
>> + */
>>  if (d_unlinked(dentry))
>>  return -EIO;
>>  
>> @@ -98,7 +125,7 @@ EXPORT_SYMBOL_GPL(debugfs_file_get);
>>   */
>>  void debugfs_file_put(struct dentry *dentry)
>>  {
>> -struct debugfs_fsdata *fsd = dentry->d_fsdata;
>> +struct debugfs_fsdata *fsd = READ_ONCE(dentry->d_fsdata);
>>  
>>  if (refcount_dec_and_test(>active_users))
>>  complete(>active_users_drained);
>> @@ -109,10 +136,11 @@ static int open_proxy_open(struct inode *inode,
>> struct file *filp)
>>  {
>>  struct dentry *dentry = F_DENTRY(filp);
>>  const struct file_operations *real_fops = NULL;
>> -int r = 0;
>> +int r;
>>  
>> -if (debugfs_file_get(dentry))
>> -return -ENOENT;
>> +r = debugfs_file_get(dentry);
>> +if (r)
>> +return r == -EIO ? -ENOENT : r;
>>  
>>  real_fops = debugfs_real_fops(filp);
>>  real_fops = fops_get(real_fops);
>> @@ -233,10 +261,11 @@ static int full_proxy_open(struct inode *inode,
>> struct file *filp)
>>  struct dentry *dentry = F_DENTRY(filp);
>>  const struct file_operations *real_fops = NULL;
>>  struct file_operations *proxy_fops = NULL;
>> -int r = 0;
>> +int r;
>>  
>> -if (debugfs_file_get(dentry))
>> -return -ENOENT;
>> +r = debugfs_file_get(dentry);
>> +if 

Re: [RFC PATCH 8/9] debugfs: defer debugfs_fsdata allocation to first usage

[Johannes Berg]

On Sun, 2017-04-16 at 11:51 +0200, Nicolai Stange wrote:
> 
> +++ b/fs/debugfs/file.c
> @@ -53,6 +53,7 @@ const struct file_operations
> *debugfs_real_fops(const struct file *filp)
>  {
>   struct debugfs_fsdata *fsd = F_DENTRY(filp)->d_fsdata;
>  
> + WARN_ON((unsigned long)fsd &
> DEBUGFS_FSDATA_IS_REAL_FOPS_BIT);
>   return fsd->real_fops;

I'm not a fan of BUG_ON(), but in this case, if you have a completely
bogus pointer here, and then you return fsd->real_fops which will be
even more bogus, and *then* you call a function from within it... that
seems like a recipe for disaster.

So either you could return some valid ops (perhaps
debugfs_noop_file_operations although those don't have .name or .poll,
so it doesn't cover everything), or you can just BUG_ON() here
directly, saving the incomprehensible crash later.

johannes

>  EXPORT_SYMBOL_GPL(debugfs_real_fops);
> @@ -74,9 +75,35 @@ EXPORT_SYMBOL_GPL(debugfs_real_fops);
>   */
>  int debugfs_file_get(struct dentry *dentry)
>  {
> - struct debugfs_fsdata *fsd = dentry->d_fsdata;
> + struct debugfs_fsdata *fsd;
> + void *d_fsd;
> +
> + d_fsd = READ_ONCE(dentry->d_fsdata);
> + if (!((unsigned long)d_fsd &
> DEBUGFS_FSDATA_IS_REAL_FOPS_BIT)) {
> + fsd = d_fsd;
> + } else {
> + fsd = kmalloc(sizeof(*fsd), GFP_KERNEL);
> + if (!fsd)
> + return -ENOMEM;
> +
> + fsd->real_fops = (void *)((unsigned long)d_fsd &
> + ~DEBUGFS_FSDATA_IS_REAL_FOPS
> _BIT);
> + refcount_set(>active_users, 1);
> + init_completion(>active_users_drained);
> + if (cmpxchg(>d_fsdata, d_fsd, fsd) != d_fsd)
> {
> + kfree(fsd);
> + fsd = READ_ONCE(dentry->d_fsdata);
> + }
> + }
>  
> - /* Avoid starvation of removers. */
> + /*
> +  * In case of a successful cmpxchg() above, this check is
> +  * strictly necessary and must follow it, see the comment in
> +  * __debugfs_remove_file().
> +  * OTOH, if the cmpxchg() hasn't been executed or wasn't
> +  * successful, this serves the purpose of not starving
> +  * removers.
> +  */
>   if (d_unlinked(dentry))
>   return -EIO;
>  
> @@ -98,7 +125,7 @@ EXPORT_SYMBOL_GPL(debugfs_file_get);
>   */
>  void debugfs_file_put(struct dentry *dentry)
>  {
> - struct debugfs_fsdata *fsd = dentry->d_fsdata;
> + struct debugfs_fsdata *fsd = READ_ONCE(dentry->d_fsdata);
>  
>   if (refcount_dec_and_test(>active_users))
>   complete(>active_users_drained);
> @@ -109,10 +136,11 @@ static int open_proxy_open(struct inode *inode,
> struct file *filp)
>  {
>   struct dentry *dentry = F_DENTRY(filp);
>   const struct file_operations *real_fops = NULL;
> - int r = 0;
> + int r;
>  
> - if (debugfs_file_get(dentry))
> - return -ENOENT;
> + r = debugfs_file_get(dentry);
> + if (r)
> + return r == -EIO ? -ENOENT : r;
>  
>   real_fops = debugfs_real_fops(filp);
>   real_fops = fops_get(real_fops);
> @@ -233,10 +261,11 @@ static int full_proxy_open(struct inode *inode,
> struct file *filp)
>   struct dentry *dentry = F_DENTRY(filp);
>   const struct file_operations *real_fops = NULL;
>   struct file_operations *proxy_fops = NULL;
> - int r = 0;
> + int r;
>  
> - if (debugfs_file_get(dentry))
> - return -ENOENT;
> + r = debugfs_file_get(dentry);
> + if (r)
> + return r == -EIO ? -ENOENT : r;
>  
>   real_fops = debugfs_real_fops(filp);
>   real_fops = fops_get(real_fops);
> diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
> index 5550f11d60bd..2360c17ec00a 100644
> --- a/fs/debugfs/inode.c
> +++ b/fs/debugfs/inode.c
> @@ -184,7 +184,10 @@ static const struct super_operations
> debugfs_super_operations = {
>  
>  static void debugfs_release_dentry(struct dentry *dentry)
>  {
> - kfree(dentry->d_fsdata);
> + void *fsd = dentry->d_fsdata;
> +
> + if (!((unsigned long)fsd & DEBUGFS_FSDATA_IS_REAL_FOPS_BIT))
> + kfree(dentry->d_fsdata);
>  }
>  
>  static struct vfsmount *debugfs_automount(struct path *path)
> @@ -346,35 +349,25 @@ static struct dentry
> *__debugfs_create_file(const char *name, umode_t mode,
>  {
>   struct dentry *dentry;
>   struct inode *inode;
> - struct debugfs_fsdata *fsd;
> -
> - fsd = kmalloc(sizeof(*fsd), GFP_KERNEL);
> - if (!fsd)
> - return NULL;
>  
>   if (!(mode & S_IFMT))
>   mode |= S_IFREG;
>   BUG_ON(!S_ISREG(mode));
>   dentry = start_creating(name, parent);
>  
> - if (IS_ERR(dentry)) {
> - kfree(fsd);
> + if (IS_ERR(dentry))
>   return NULL;
> - }
>  
>   inode = debugfs_get_inode(dentry->d_sb);
> - if (unlikely(!inode)) {
> - kfree(fsd);
> + if (unlikely(!inode))
>    

Re: [RFC PATCH 8/9] debugfs: defer debugfs_fsdata allocation to first usage

[Johannes Berg]

On Sun, 2017-04-16 at 11:51 +0200, Nicolai Stange wrote:
> 
> +++ b/fs/debugfs/file.c
> @@ -53,6 +53,7 @@ const struct file_operations
> *debugfs_real_fops(const struct file *filp)
>  {
>   struct debugfs_fsdata *fsd = F_DENTRY(filp)->d_fsdata;
>  
> + WARN_ON((unsigned long)fsd &
> DEBUGFS_FSDATA_IS_REAL_FOPS_BIT);
>   return fsd->real_fops;

I'm not a fan of BUG_ON(), but in this case, if you have a completely
bogus pointer here, and then you return fsd->real_fops which will be
even more bogus, and *then* you call a function from within it... that
seems like a recipe for disaster.

So either you could return some valid ops (perhaps
debugfs_noop_file_operations although those don't have .name or .poll,
so it doesn't cover everything), or you can just BUG_ON() here
directly, saving the incomprehensible crash later.

johannes

>  EXPORT_SYMBOL_GPL(debugfs_real_fops);
> @@ -74,9 +75,35 @@ EXPORT_SYMBOL_GPL(debugfs_real_fops);
>   */
>  int debugfs_file_get(struct dentry *dentry)
>  {
> - struct debugfs_fsdata *fsd = dentry->d_fsdata;
> + struct debugfs_fsdata *fsd;
> + void *d_fsd;
> +
> + d_fsd = READ_ONCE(dentry->d_fsdata);
> + if (!((unsigned long)d_fsd &
> DEBUGFS_FSDATA_IS_REAL_FOPS_BIT)) {
> + fsd = d_fsd;
> + } else {
> + fsd = kmalloc(sizeof(*fsd), GFP_KERNEL);
> + if (!fsd)
> + return -ENOMEM;
> +
> + fsd->real_fops = (void *)((unsigned long)d_fsd &
> + ~DEBUGFS_FSDATA_IS_REAL_FOPS
> _BIT);
> + refcount_set(>active_users, 1);
> + init_completion(>active_users_drained);
> + if (cmpxchg(>d_fsdata, d_fsd, fsd) != d_fsd)
> {
> + kfree(fsd);
> + fsd = READ_ONCE(dentry->d_fsdata);
> + }
> + }
>  
> - /* Avoid starvation of removers. */
> + /*
> +  * In case of a successful cmpxchg() above, this check is
> +  * strictly necessary and must follow it, see the comment in
> +  * __debugfs_remove_file().
> +  * OTOH, if the cmpxchg() hasn't been executed or wasn't
> +  * successful, this serves the purpose of not starving
> +  * removers.
> +  */
>   if (d_unlinked(dentry))
>   return -EIO;
>  
> @@ -98,7 +125,7 @@ EXPORT_SYMBOL_GPL(debugfs_file_get);
>   */
>  void debugfs_file_put(struct dentry *dentry)
>  {
> - struct debugfs_fsdata *fsd = dentry->d_fsdata;
> + struct debugfs_fsdata *fsd = READ_ONCE(dentry->d_fsdata);
>  
>   if (refcount_dec_and_test(>active_users))
>   complete(>active_users_drained);
> @@ -109,10 +136,11 @@ static int open_proxy_open(struct inode *inode,
> struct file *filp)
>  {
>   struct dentry *dentry = F_DENTRY(filp);
>   const struct file_operations *real_fops = NULL;
> - int r = 0;
> + int r;
>  
> - if (debugfs_file_get(dentry))
> - return -ENOENT;
> + r = debugfs_file_get(dentry);
> + if (r)
> + return r == -EIO ? -ENOENT : r;
>  
>   real_fops = debugfs_real_fops(filp);
>   real_fops = fops_get(real_fops);
> @@ -233,10 +261,11 @@ static int full_proxy_open(struct inode *inode,
> struct file *filp)
>   struct dentry *dentry = F_DENTRY(filp);
>   const struct file_operations *real_fops = NULL;
>   struct file_operations *proxy_fops = NULL;
> - int r = 0;
> + int r;
>  
> - if (debugfs_file_get(dentry))
> - return -ENOENT;
> + r = debugfs_file_get(dentry);
> + if (r)
> + return r == -EIO ? -ENOENT : r;
>  
>   real_fops = debugfs_real_fops(filp);
>   real_fops = fops_get(real_fops);
> diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
> index 5550f11d60bd..2360c17ec00a 100644
> --- a/fs/debugfs/inode.c
> +++ b/fs/debugfs/inode.c
> @@ -184,7 +184,10 @@ static const struct super_operations
> debugfs_super_operations = {
>  
>  static void debugfs_release_dentry(struct dentry *dentry)
>  {
> - kfree(dentry->d_fsdata);
> + void *fsd = dentry->d_fsdata;
> +
> + if (!((unsigned long)fsd & DEBUGFS_FSDATA_IS_REAL_FOPS_BIT))
> + kfree(dentry->d_fsdata);
>  }
>  
>  static struct vfsmount *debugfs_automount(struct path *path)
> @@ -346,35 +349,25 @@ static struct dentry
> *__debugfs_create_file(const char *name, umode_t mode,
>  {
>   struct dentry *dentry;
>   struct inode *inode;
> - struct debugfs_fsdata *fsd;
> -
> - fsd = kmalloc(sizeof(*fsd), GFP_KERNEL);
> - if (!fsd)
> - return NULL;
>  
>   if (!(mode & S_IFMT))
>   mode |= S_IFREG;
>   BUG_ON(!S_ISREG(mode));
>   dentry = start_creating(name, parent);
>  
> - if (IS_ERR(dentry)) {
> - kfree(fsd);
> + if (IS_ERR(dentry))
>   return NULL;
> - }
>  
>   inode = debugfs_get_inode(dentry->d_sb);
> - if (unlikely(!inode)) {
> - kfree(fsd);
> + if (unlikely(!inode))
>    

[RFC PATCH 8/9] debugfs: defer debugfs_fsdata allocation to first usage

[Nicolai Stange]

Currently, __debugfs_create_file allocates one struct debugfs_fsdata
instance for every file created. However, there are potentially many
debugfs file around, most of which are never touched by userspace.

Thus, defer the allocations to the first usage, i.e. to the first
debugfs_file_get().

A dentry's ->d_fsdata starts out to point to the "real", user provided
fops. After a debugfs_fsdata instance has been allocated (and the real
fops pointer has been moved over into its ->real_fops member),
->d_fsdata is changed to point to it from then on. The two cases are
distinguished by setting BIT(0) for the real fops case.

struct debugfs_fsdata's foremost purpose is to track active users and to
make debugfs_remove() block until they are done. Since no debugfs_fsdata
instance means no active users, make debugfs_remove() return immediately
in this case.

Take care of possible races between debugfs_file_get() and
debugfs_remove(): either debugfs_remove() must see a debugfs_fsdata
instance and thus wait for possible active users or debugfs_file_get() must
see a dead dentry and return immediately.

Make a dentry's ->d_release(), i.e. debugfs_release_dentry(), check whether
->d_fsdata is actually a debugfs_fsdata instance before kfree()ing it.

Finally, the set of possible error codes returned from debugfs_file_get()
has grown from -EIO to -EIO and -ENOMEM. Make open_proxy_open() and
full_proxy_open() pass the -ENOMEM onwards to their callers.

Signed-off-by: Nicolai Stange 
---
 fs/debugfs/file.c | 47 ++-
 fs/debugfs/inode.c| 36 +++-
 fs/debugfs/internal.h |  8 
 3 files changed, 65 insertions(+), 26 deletions(-)

diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
index d92038c5f131..f4dfd7d0d625 100644
--- a/fs/debugfs/file.c
+++ b/fs/debugfs/file.c
@@ -53,6 +53,7 @@ const struct file_operations *debugfs_real_fops(const struct 
file *filp)
 {
struct debugfs_fsdata *fsd = F_DENTRY(filp)->d_fsdata;
 
+   WARN_ON((unsigned long)fsd & DEBUGFS_FSDATA_IS_REAL_FOPS_BIT);
return fsd->real_fops;
 }
 EXPORT_SYMBOL_GPL(debugfs_real_fops);
@@ -74,9 +75,35 @@ EXPORT_SYMBOL_GPL(debugfs_real_fops);
  */
 int debugfs_file_get(struct dentry *dentry)
 {
-   struct debugfs_fsdata *fsd = dentry->d_fsdata;
+   struct debugfs_fsdata *fsd;
+   void *d_fsd;
+
+   d_fsd = READ_ONCE(dentry->d_fsdata);
+   if (!((unsigned long)d_fsd & DEBUGFS_FSDATA_IS_REAL_FOPS_BIT)) {
+   fsd = d_fsd;
+   } else {
+   fsd = kmalloc(sizeof(*fsd), GFP_KERNEL);
+   if (!fsd)
+   return -ENOMEM;
+
+   fsd->real_fops = (void *)((unsigned long)d_fsd &
+   ~DEBUGFS_FSDATA_IS_REAL_FOPS_BIT);
+   refcount_set(>active_users, 1);
+   init_completion(>active_users_drained);
+   if (cmpxchg(>d_fsdata, d_fsd, fsd) != d_fsd) {
+   kfree(fsd);
+   fsd = READ_ONCE(dentry->d_fsdata);
+   }
+   }
 
-   /* Avoid starvation of removers. */
+   /*
+* In case of a successful cmpxchg() above, this check is
+* strictly necessary and must follow it, see the comment in
+* __debugfs_remove_file().
+* OTOH, if the cmpxchg() hasn't been executed or wasn't
+* successful, this serves the purpose of not starving
+* removers.
+*/
if (d_unlinked(dentry))
return -EIO;
 
@@ -98,7 +125,7 @@ EXPORT_SYMBOL_GPL(debugfs_file_get);
  */
 void debugfs_file_put(struct dentry *dentry)
 {
-   struct debugfs_fsdata *fsd = dentry->d_fsdata;
+   struct debugfs_fsdata *fsd = READ_ONCE(dentry->d_fsdata);
 
if (refcount_dec_and_test(>active_users))
complete(>active_users_drained);
@@ -109,10 +136,11 @@ static int open_proxy_open(struct inode *inode, struct 
file *filp)
 {
struct dentry *dentry = F_DENTRY(filp);
const struct file_operations *real_fops = NULL;
-   int r = 0;
+   int r;
 
-   if (debugfs_file_get(dentry))
-   return -ENOENT;
+   r = debugfs_file_get(dentry);
+   if (r)
+   return r == -EIO ? -ENOENT : r;
 
real_fops = debugfs_real_fops(filp);
real_fops = fops_get(real_fops);
@@ -233,10 +261,11 @@ static int full_proxy_open(struct inode *inode, struct 
file *filp)
struct dentry *dentry = F_DENTRY(filp);
const struct file_operations *real_fops = NULL;
struct file_operations *proxy_fops = NULL;
-   int r = 0;
+   int r;
 
-   if (debugfs_file_get(dentry))
-   return -ENOENT;
+   r = debugfs_file_get(dentry);
+   if (r)
+   return r == -EIO ? -ENOENT : r;
 
real_fops = debugfs_real_fops(filp);
real_fops = fops_get(real_fops);
diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c

[RFC PATCH 8/9] debugfs: defer debugfs_fsdata allocation to first usage

[Nicolai Stange]

Currently, __debugfs_create_file allocates one struct debugfs_fsdata
instance for every file created. However, there are potentially many
debugfs file around, most of which are never touched by userspace.

Thus, defer the allocations to the first usage, i.e. to the first
debugfs_file_get().

A dentry's ->d_fsdata starts out to point to the "real", user provided
fops. After a debugfs_fsdata instance has been allocated (and the real
fops pointer has been moved over into its ->real_fops member),
->d_fsdata is changed to point to it from then on. The two cases are
distinguished by setting BIT(0) for the real fops case.

struct debugfs_fsdata's foremost purpose is to track active users and to
make debugfs_remove() block until they are done. Since no debugfs_fsdata
instance means no active users, make debugfs_remove() return immediately
in this case.

Take care of possible races between debugfs_file_get() and
debugfs_remove(): either debugfs_remove() must see a debugfs_fsdata
instance and thus wait for possible active users or debugfs_file_get() must
see a dead dentry and return immediately.

Make a dentry's ->d_release(), i.e. debugfs_release_dentry(), check whether
->d_fsdata is actually a debugfs_fsdata instance before kfree()ing it.

Finally, the set of possible error codes returned from debugfs_file_get()
has grown from -EIO to -EIO and -ENOMEM. Make open_proxy_open() and
full_proxy_open() pass the -ENOMEM onwards to their callers.

Signed-off-by: Nicolai Stange 
---
 fs/debugfs/file.c | 47 ++-
 fs/debugfs/inode.c| 36 +++-
 fs/debugfs/internal.h |  8 
 3 files changed, 65 insertions(+), 26 deletions(-)

diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
index d92038c5f131..f4dfd7d0d625 100644
--- a/fs/debugfs/file.c
+++ b/fs/debugfs/file.c
@@ -53,6 +53,7 @@ const struct file_operations *debugfs_real_fops(const struct 
file *filp)
 {
struct debugfs_fsdata *fsd = F_DENTRY(filp)->d_fsdata;
 
+   WARN_ON((unsigned long)fsd & DEBUGFS_FSDATA_IS_REAL_FOPS_BIT);
return fsd->real_fops;
 }
 EXPORT_SYMBOL_GPL(debugfs_real_fops);
@@ -74,9 +75,35 @@ EXPORT_SYMBOL_GPL(debugfs_real_fops);
  */
 int debugfs_file_get(struct dentry *dentry)
 {
-   struct debugfs_fsdata *fsd = dentry->d_fsdata;
+   struct debugfs_fsdata *fsd;
+   void *d_fsd;
+
+   d_fsd = READ_ONCE(dentry->d_fsdata);
+   if (!((unsigned long)d_fsd & DEBUGFS_FSDATA_IS_REAL_FOPS_BIT)) {
+   fsd = d_fsd;
+   } else {
+   fsd = kmalloc(sizeof(*fsd), GFP_KERNEL);
+   if (!fsd)
+   return -ENOMEM;
+
+   fsd->real_fops = (void *)((unsigned long)d_fsd &
+   ~DEBUGFS_FSDATA_IS_REAL_FOPS_BIT);
+   refcount_set(>active_users, 1);
+   init_completion(>active_users_drained);
+   if (cmpxchg(>d_fsdata, d_fsd, fsd) != d_fsd) {
+   kfree(fsd);
+   fsd = READ_ONCE(dentry->d_fsdata);
+   }
+   }
 
-   /* Avoid starvation of removers. */
+   /*
+* In case of a successful cmpxchg() above, this check is
+* strictly necessary and must follow it, see the comment in
+* __debugfs_remove_file().
+* OTOH, if the cmpxchg() hasn't been executed or wasn't
+* successful, this serves the purpose of not starving
+* removers.
+*/
if (d_unlinked(dentry))
return -EIO;
 
@@ -98,7 +125,7 @@ EXPORT_SYMBOL_GPL(debugfs_file_get);
  */
 void debugfs_file_put(struct dentry *dentry)
 {
-   struct debugfs_fsdata *fsd = dentry->d_fsdata;
+   struct debugfs_fsdata *fsd = READ_ONCE(dentry->d_fsdata);
 
if (refcount_dec_and_test(>active_users))
complete(>active_users_drained);
@@ -109,10 +136,11 @@ static int open_proxy_open(struct inode *inode, struct 
file *filp)
 {
struct dentry *dentry = F_DENTRY(filp);
const struct file_operations *real_fops = NULL;
-   int r = 0;
+   int r;
 
-   if (debugfs_file_get(dentry))
-   return -ENOENT;
+   r = debugfs_file_get(dentry);
+   if (r)
+   return r == -EIO ? -ENOENT : r;
 
real_fops = debugfs_real_fops(filp);
real_fops = fops_get(real_fops);
@@ -233,10 +261,11 @@ static int full_proxy_open(struct inode *inode, struct 
file *filp)
struct dentry *dentry = F_DENTRY(filp);
const struct file_operations *real_fops = NULL;
struct file_operations *proxy_fops = NULL;
-   int r = 0;
+   int r;
 
-   if (debugfs_file_get(dentry))
-   return -ENOENT;
+   r = debugfs_file_get(dentry);
+   if (r)
+   return r == -EIO ? -ENOENT : r;
 
real_fops = debugfs_real_fops(filp);
real_fops = fops_get(real_fops);
diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
index