Re: git: 9a7d03c7df35 - main - sendfile: cover the entire sendfile operation under CURVNET_SET()

On Tue, May 6, 2025 at 12:34 PM Gleb Smirnoff <glebius@freebsd.org> wrote:
>
> The branch main has been updated by glebius:
>
> URL: https://cgit.FreeBSD.org/src/commit/?id=9a7d03c7df3536cdb5faf0848c6dab4a5d7bcef8
>
> commit 9a7d03c7df3536cdb5faf0848c6dab4a5d7bcef8
> Author:     Gleb Smirnoff <glebius@FreeBSD.org>
> AuthorDate: 2025-05-06 18:15:15 +0000
> Commit:     Gleb Smirnoff <glebius@FreeBSD.org>
> CommitDate: 2025-05-06 19:34:26 +0000
>
>     sendfile: cover the entire sendfile operation under CURVNET_SET()
>
>     There is no reason to set/restore for every single pr_send(), as it never
>     changes.  Also, cover call into pr_sendfile_wait with CURVNET_SET() fixing
>     recent regression in unix(4).
>
>     Now we would enter sendfile_iodone() with curvnet set, when called
>     synchronously.  Although, it is easy to tell a syncronous call from I/O
>     completion, unfortunately the vnet(9) macros do not support conditional
>     invocation, so just change CURVNET_SET() to CURVNET_SET_QUIET() and add a
>     comment that we are aware of the recursion.

Seeing a build error on main with this

--- kern_sendfile.o ---
/usr/src/sys/kern/kern_sendfile.c:788:6: error: variable 'saved_vnet'
is used uninitialized whenever 'if' condition is true [
-Werror,-Wsometimes-uninitialized]
 788 |         if (error != 0)
     |             ^~~~~~~~~~
/usr/src/sys/kern/kern_sendfile.c:1273:2: note: uninitialized use occurs here
1273 |         CURVNET_RESTORE();

and

/usr/src/sys/kern/kern_sendfile.c:788:2: note: remove the 'if' if its
condition is always false
 788 |         if (error != 0)
     |         ^~~~~~~~~~~~~~~
 789 |                 goto out;
     |                 ~~~~~~~~




>     Reported-by:    syzbot+7b0b20cf2c672c181d98@syzkaller.appspotmail.com
> ---
>  sys/kern/kern_sendfile.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/sys/kern/kern_sendfile.c b/sys/kern/kern_sendfile.c
> index 1fd1828c37c7..c428d80e0e1a 100644
> --- a/sys/kern/kern_sendfile.c
> +++ b/sys/kern/kern_sendfile.c
> @@ -285,6 +285,11 @@ sendfile_iowait(struct sf_io *sfio, const char *wmesg)
>
>  /*
>   * I/O completion callback.
> + *
> + * When called via I/O path, the curvnet is not set and should be obtained
> + * from the socket.  When called synchronously from vn_sendfile(), usually
> + * to report error or just release the reference (all pages are valid), then
> + * curvnet shall be already set.
>   */
>  static void
>  sendfile_iodone(void *arg, vm_page_t *pa, int count, int error)
> @@ -365,7 +370,7 @@ sendfile_iodone(void *arg, vm_page_t *pa, int count, int error)
>                     ("non-ext_pgs mbuf with TLS session"));
>  #endif
>         so = sfio->so;
> -       CURVNET_SET(so->so_vnet);
> +       CURVNET_SET_QUIET(so->so_vnet);
>         if (__predict_false(sfio->error)) {
>                 /*
>                  * I/O operation failed.  The state of data in the socket
> @@ -782,6 +787,7 @@ vn_sendfile(struct file *fp, int sockfd, struct uio *hdr_uio,
>         error = sendfile_getsock(td, sockfd, &sock_fp, &so);
>         if (error != 0)
>                 goto out;
> +       CURVNET_SET(so->so_vnet);
>         pr = so->so_proto;
>
>  #ifdef MAC
> @@ -1161,7 +1167,6 @@ prepend_header:
>                     ("%s: mlen %u space %d hdrlen %d",
>                     __func__, m_length(m, NULL), space, hdrlen));
>
> -               CURVNET_SET(so->so_vnet);
>  #ifdef KERN_TLS
>                 if (tls != NULL)
>                         ktls_frame(m, tls, &tls_enq_cnt, TLS_RLTYPE_APP);
> @@ -1203,8 +1208,6 @@ prepend_header:
>                         tcp_log_sendfile(so, offset, nbytes, flags);
>                 }
>  #endif
> -               CURVNET_RESTORE();
> -
>                 m = NULL;
>                 if (error)
>                         goto done;
> @@ -1265,9 +1268,9 @@ out:
>         if (tls != NULL)
>                 ktls_free(tls);
>  #endif
> -
>         if (error == ERESTART)
>                 error = EINTR;
> +       CURVNET_RESTORE();
>
>         return (error);
>  }