> On Sep 30, 2022, at 11:02 PM, Robert Elz <kre%munnari.OZ.AU@localhost> wrote:
>
> Date: Fri, 30 Sep 2022 20:15:07 -0400
> From: Christos Zoulas <christos%zoulas.com@localhost>
> Message-ID: <AB0EA4EE-C3B9-4237-BCA3-C1672EB4FD8A%zoulas.com@localhost>
>
> | It does not need an extra flag (it looks in the file descriptor flags to
> | find if it needs to set or not.
>
> One of us is confused. From where in this case does anything
> get the exclose flag set? That's the whole question here. The
> flags arg that is passed around has O_CLOEXEC set in it - you used
> that in the call to fd_set_exclose() in kern/tty_ptm.c ... but where
> you said that would be better done in fd_affix().
This is what I meant:
RCS file: /cvsroot/src/sys/kern/kern_descrip.c,v
retrieving revision 1.251
diff -u -u -r1.251 kern_descrip.c
--- kern_descrip.c 29 Jun 2021 22:40:53 -0000 1.251
+++ kern_descrip.c 1 Oct 2022 16:56:44 -0000
@@ -1162,6 +1162,7 @@
KASSERT(ff->ff_allocated);
KASSERT(fd_isused(fdp, fd));
KASSERT(fd >= NDFDFILE || ff == (fdfile_t *)fdp->fd_dfdfile[fd]);
+ ff->ff_exclose = (fp->f_flag & O_CLOEXEC) != 0;
/* No need to lock in order to make file initially visible. */
ff->ff_file = fp;
>
> That does not have access to the flags. So from where is it going
> to get the close on exec info ?
>
> My reading of do_open() is that the O_CLOEXEC flag is never even
> examined when a cloning device is opened, it doesn't get set on
> the original fd (the cloner) or the cloned device (other than by
> your recent modification for /dev/pmx).
>
> Did I misread the code?
>
> Or are you planning something different than it seemed?
>
> | to find other cases where we forgot to call fd_set_exclose() before calling
> | fd_affix().
>
> My point is that it should not be necessary to call fd_set_exclose()
> in every (or any) cloning device driver. The open syscall handling
> is where that should be done, just as it is for all the opens that
> are not cloning devices.
What does it mean when the open specifies O_CLOEXEC
and ff->ff_exclose is false? Can that happen? Is that desirable?
> Why be different?
>
> | It also does not need locking because the process can't access
> | the descriptor before calling fd_affix.
>
> The locking I was referring to are the vnode locks/references in
> do_open(), not anything related to the file struct or descriptor.
> I just do not feel competent to get all of that correct in this
> case (more complex than the normal case because of the extra vnode
> involved) and would prefer if someone familiar with all of that
> were to handle it - particularly in the extra error case that will
> need to be handled, even if I cannot see how it would actually fire
> in the case in question.
I am fine with the locking to stay where it is. I guess it is probably
not needed after dup/clone, since the underlying vnode is shared...
christos
Attachment:
signature.asc
Description: Message signed with OpenPGP