During F_SETFL, don't change file flags on error

[asomers]

Summary

Previously, event if the FIONBIO or FIOASYNC ioctl failed, the file's
f_flags variable would still be changed. Now, kern_fcntl will restore
the original flags if the ioctl fails.

PR: 265736
Reported by: Yuval Pavel Zholkover <[e-mail]>
MFC after: 2 weeks

Test Plan

Test case is in Bugzilla

[kib]

May be, set the flags after fo_ioctls ops? Then you do not need to revert.

This would also slightly reduce a window where e.g. two threads do setflags, one ioctl fails, and the thread reverts flags set by the successful thread.

[asomers]

In D40955#932173, @kib wrote:

May be, set the flags after fo_ioctls ops? Then you do not need to revert.

I thought about that. However, the fp->f_flag is accessed by fo_ioctls. For example, in vn_ioctl. So the flag has to be set before.

This would also slightly reduce a window where e.g. two threads do setflags, one ioctl fails, and the thread reverts flags set by the successful thread.

I think we should be ok in this case, because the revert_f_setfl block will only revert those flags that were actually changed above. Atomic instructions make it possible for us to know which flags were changed.

[kib]

In D40955#932178, @asomers wrote:

In D40955#932173, @kib wrote:

This would also slightly reduce a window where e.g. two threads do setflags, one ioctl fails, and the thread reverts flags set by the successful thread.

I think we should be ok in this case, because the revert_f_setfl block will only revert those flags that were actually changed above. Atomic instructions make it possible for us to know which flags were changed.

Other thread might see that the needed flag is set, and then it is reverted.

[asomers]

sys/kern/kern_descrip.c

590–597

BTW @kib do you know why this line is here? It seems to be "if FIOASYNC fails, then turn off non-blocking mode". Why would that be? This logic has been in place ever since 1991. https://svnweb.freebsd.org/csrg/sys/kern/kern_descrip.c?r1=48028&r2=48029&

[kib]

sys/kern/kern_descrip.c

590–597

I suspect it is a too rough attempt to revert the action. Perhaps more fine-grained unwind on error would be applicable, as you noted.