Re: [PATCH v2 1/3] pidfd: extend pidfd_get_pid() and de-duplicate pid lookup

On Wed, Oct 16, 2024 at 01:16:15AM -0700, Suren Baghdasaryan wrote:

On Tue, Oct 15, 2024 at 11:05 PM Lorenzo Stoakes lorenzo.stoakes@oracle.com wrote:

...

On Tue, Oct 15, 2024 at 12:40:41PM -0700, Suren Baghdasaryan wrote: [snip]

...

...

-struct pid *pidfd_get_pid(unsigned int fd, unsigned int *flags) +struct pid *__pidfd_get_pid(unsigned int pidfd, bool pin_pid,

•                       bool allow_proc, unsigned int *flags,
  

•                       struct fd *fd)
  

{

•   struct fd f;
  

•   struct file *file;
    struct pid *pid;
  

•   struct fd f = fdget(pidfd);
  
  

•   f = fdget(fd);
  

•   if (!fd_file(f))
  

•   file = fd_file(f);
  

•   if (!file)
            return ERR_PTR(-EBADF);
  
  

•   pid = pidfd_pid(fd_file(f));
  

•   if (!IS_ERR(pid)) {
  

•           get_pid(pid);
  

•           *flags = fd_file(f)->f_flags;
  

•   pid = pidfd_pid(file);
  

•   /* If we allow opening a pidfd via /proc/<pid>, do so. */
  

•   if (IS_ERR(pid) && allow_proc)
  

•           pid = tgid_pidfd_to_pid(file);
  

•   if (IS_ERR(pid)) {
  

•           fdput(f);
  

•           return pid;
    }
  
  

•   fdput(f);
  

•   if (pin_pid)
  

•           get_pid(pid);
  

•   else
  

•           WARN_ON_ONCE(!fd); /* Nothing to keep pid/pidfd around? */
  

•   if (flags)
  

•           *flags = file->f_flags;
  

•   /*
  

•    * If the user provides an fd output then it will handle decrementing
  

•    * its reference counter.
  

•    */
  

•   if (fd)
  

•           *fd = f;
  

•   else
  

•           /* Otherwise we release it. */
  

•           fdput(f);
  

•   return pid;
  

}

There is an EXPORT_SYMBOL_GPL(pidfd_get_pid) right after this line. It should also be changed to EXPORT_SYMBOL_GPL(__pidfd_get_pid), otherwise __pidfd_get_pid() will not be exported. A module calling pidfd_get_pid() now inlined in the header file will try to call __pidfd_get_pid() and will have trouble resolving this symbol.

Hmm hang on not there isn't? I don't see that anywhere?

Doh! Sorry, I didn't realize the export was an out-of-tree Android change. Never mind...

No probs :P just glad I didn't miss something in this series!

Hey maybe a motivation to upstream some of this? ;)

...

[snip]