Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1272536 > unrolled thread

Re: Anonymous inode cleanup?

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: Anonymous inode cleanup? Rajat Jain <rajatxjain@gmail.com> - 2015-11-18 21:30 +0100
    Re: Anonymous inode cleanup? Al Viro <viro@ZenIV.linux.org.uk> - 2015-11-18 22:10 +0100
      Re: Anonymous inode cleanup? Rajat Jain <rajatxjain@gmail.com> - 2015-11-18 22:20 +0100
        Re: Anonymous inode cleanup? Al Viro <viro@ZenIV.linux.org.uk> - 2015-11-18 22:30 +0100
          Re: Anonymous inode cleanup? Rajat Jain <rajatxjain@gmail.com> - 2015-11-18 22:30 +0100

#1272536 — Re: Anonymous inode cleanup?

Hi Al,

Thanks a lot! That was very helpful. I have one follow up question though.

On Thu, Nov 12, 2015 at 10:31 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Thu, Nov 12, 2015 at 09:43:00PM -0800, Rajat Jain wrote:
>> Hello,
>>
>> I'm writing a module that wants to get anonymous fd [using
>> anon_inode_getfd()] and my code looks like this:
>>
>> fd = anon_inode_getfd(...)
>> if (fd < 0)
>>     return -EINVAL;
>>
>> if (foobar_fail()) {
>>     /* undo everything */
>>     return -EINVAL;
>> }
>>
>> My question is that in case of a failure after the anon_inode_getfd(),
>> I want to cleanup and undo whatever needs to be done w.r.t. anodnymous
>> fd I just allocated. (May be put a reference, or return the fd to the
>> free pool or whatever). Can some one please let me know what cleanup
>> needs to be done?
>>
>> However neither I see a cleanup function, nor I see any of the drivers
>> attempting
>> to free the fd in case of failure.
>
> You can't.  As soon as it's in descriptor table, you'd better be *done*
> with it.  No "I need more setup done", no "I just need to do one final
> check" - the moment it hits the descriptor table, another thread might
> be issuing syscalls on it.  Including dup2(), so there's no way to take
> it back.  Moreover, another thread might've done dup2() over your
> descriptor, so you can't even decide to close the one you'd just installed.
> Yes, even in cases when the failed action would be to report the resulting
> descriptor to userland.  Generally you should try to return descriptors to
> userland only via the syscall return value.
>
> _If_ you are returning them via a sucky API, the right sequence is
>         reserve the descriptor(s)
>         set the file(s) up
>         fill whatever structure you'll be using to report descriptors to
> userland and copy it to userland memory

If this step fails, what is the cleanup needed for "set the files up"
anon_inode_getfile() step? is it fput()?

Thanks,

Rajat

>         use fd_install() to put files into descriptor table.
>
> See e.g. fs/pipe.c and look for pipe2 in there for example of dealing with
> such APIs.
>
> "Set the file up" primitive in case of anon_inode is anon_inode_getfile();
> grep and you shall see...
>
> Again, fd_install() is the equivalent of hitting "send" - there's no way
> to make what you've published disappear.  It's the point of no return.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1272571

On Wed, Nov 18, 2015 at 12:28:38PM -0800, Rajat Jain wrote:

> > _If_ you are returning them via a sucky API, the right sequence is
> >         reserve the descriptor(s)
> >         set the file(s) up
> >         fill whatever structure you'll be using to report descriptors to
> > userland and copy it to userland memory
> 
> If this step fails, what is the cleanup needed for "set the files up"
> anon_inode_getfile() step? is it fput()?

Yes.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1272581

I see reference taken for the path and module in anon_inode_getfile()

Don't I need to drop that, or would fput() do it for me?

        path_put(&path);
        module_put(fops->owner);

On Wed, Nov 18, 2015 at 1:06 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Wed, Nov 18, 2015 at 12:28:38PM -0800, Rajat Jain wrote:
>
>> > _If_ you are returning them via a sucky API, the right sequence is
>> >         reserve the descriptor(s)
>> >         set the file(s) up
>> >         fill whatever structure you'll be using to report descriptors to
>> > userland and copy it to userland memory
>>
>> If this step fails, what is the cleanup needed for "set the files up"
>> anon_inode_getfile() step? is it fput()?
>
> Yes.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1272590

On Wed, Nov 18, 2015 at 01:10:10PM -0800, Rajat Jain wrote:
> I see reference taken for the path and module in anon_inode_getfile()
> 
> Don't I need to drop that, or would fput() do it for me?
> 
>         path_put(&path);

        dput(dentry);
        mntput(mnt);
in the very end of __fput()

>         module_put(fops->owner);

        fops_put(file->f_op);
slightly earlier in the same function.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1272593

Thanks a lot!

On Wed, Nov 18, 2015 at 1:24 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Wed, Nov 18, 2015 at 01:10:10PM -0800, Rajat Jain wrote:
>> I see reference taken for the path and module in anon_inode_getfile()
>>
>> Don't I need to drop that, or would fput() do it for me?
>>
>>         path_put(&path);
>
>         dput(dentry);
>         mntput(mnt);
> in the very end of __fput()
>
>>         module_put(fops->owner);
>
>         fops_put(file->f_op);
> slightly earlier in the same function.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web