On Tue, Jan 31, 2023 at 09:50:26PM +0300, Vitaliy Makkoveev wrote:
> On Tue, Jan 31, 2023 at 06:00:45PM +0000, Visa Hankala wrote:
> > On Tue, Jan 31, 2023 at 12:44:47PM +0300, Vitaliy Makkoveev wrote:
> > > Since we have soalloc() to do common socket initialization, move the
> > > rest within. I mostly need to do this because standalone socket's buffer
> > > locking require to introduce another klistops data for buffers and there
> > > is no reason to add more copypaste to sonewconn().
> > >
> > > Also this makes `socket_klistops' private to kern/uipc_socket.c
> > >
> > > @@ -226,9 +225,6 @@ sonewconn(struct socket *head, int conns
> > > so->so_rcv.sb_lowat = head->so_rcv.sb_lowat;
> > > so->so_rcv.sb_timeo_nsecs = head->so_rcv.sb_timeo_nsecs;
> > >
> > > - klist_init(&so->so_rcv.sb_klist, &socket_klistops, so);
> > > - klist_init(&so->so_snd.sb_klist, &socket_klistops, so);
> > > - sigio_init(&so->so_sigio);
> > > sigio_copy(&so->so_sigio, &head->so_sigio);
> >
> > With this change, something should call klist_free() and sigio_free()
> > for 'so' if soreserve() fails in sonewconn().
> >
>
> klist_init() and sigio_init() alloc nothing, but for consistency reason
> they shold.
>
> I like to do this in the combined error path for soneconn() and
> pru_attach().
>
> Index: sys/kern/uipc_socket.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/uipc_socket.c,v
> retrieving revision 1.299
> diff -u -p -r1.299 uipc_socket.c
> --- sys/kern/uipc_socket.c 27 Jan 2023 21:01:59 -0000 1.299
> +++ sys/kern/uipc_socket.c 31 Jan 2023 18:44:57 -0000
> @@ -112,6 +112,16 @@ const struct filterops soexcept_filtops
> .f_process = filt_soprocess,
> };
>
> +void klist_soassertlk(void *);
> +int klist_solock(void *);
> +void klist_sounlock(void *, int);
> +
> +const struct klistops socket_klistops = {
> + .klo_assertlk = klist_soassertlk,
> + .klo_lock = klist_solock,
> + .klo_unlock = klist_sounlock,
> +};
> +
> #ifndef SOMINCONN
> #define SOMINCONN 80
> #endif /* SOMINCONN */
> @@ -148,6 +158,11 @@ soalloc(int wait)
> return (NULL);
> rw_init_flags(&so->so_lock, "solock", RWL_DUPOK);
> refcnt_init(&so->so_refcnt);
> + klist_init(&so->so_rcv.sb_klist, &socket_klistops, so);
> + klist_init(&so->so_snd.sb_klist, &socket_klistops, so);
> + sigio_init(&so->so_sigio);
> + TAILQ_INIT(&so->so_q0);
> + TAILQ_INIT(&so->so_q);
>
> return (so);
> }
> @@ -176,11 +191,6 @@ socreate(int dom, struct socket **aso, i
> if (prp->pr_type != type)
> return (EPROTOTYPE);
> so = soalloc(M_WAIT);
> - klist_init(&so->so_rcv.sb_klist, &socket_klistops, so);
> - klist_init(&so->so_snd.sb_klist, &socket_klistops, so);
> - sigio_init(&so->so_sigio);
> - TAILQ_INIT(&so->so_q0);
> - TAILQ_INIT(&so->so_q);
> so->so_type = type;
> if (suser(p) == 0)
> so->so_state = SS_PRIV;
> @@ -2333,12 +2343,6 @@ klist_sounlock(void *arg, int ls)
>
> sounlock(so);
> }
> -
> -const struct klistops socket_klistops = {
> - .klo_assertlk = klist_soassertlk,
> - .klo_lock = klist_solock,
> - .klo_unlock = klist_sounlock,
> -};
>
> #ifdef DDB
> void
> Index: sys/kern/uipc_socket2.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/uipc_socket2.c,v
> retrieving revision 1.134
> diff -u -p -r1.134 uipc_socket2.c
> --- sys/kern/uipc_socket2.c 27 Jan 2023 18:46:34 -0000 1.134
> +++ sys/kern/uipc_socket2.c 31 Jan 2023 18:44:57 -0000
> @@ -41,7 +41,6 @@
> #include <sys/socket.h>
> #include <sys/socketvar.h>
> #include <sys/signalvar.h>
> -#include <sys/event.h>
> #include <sys/pool.h>
>
> /*
> @@ -213,12 +212,8 @@ sonewconn(struct socket *head, int conns
> /*
> * Inherit watermarks but those may get clamped in low mem situations.
> */
> - if (soreserve(so, head->so_snd.sb_hiwat, head->so_rcv.sb_hiwat)) {
> - if (persocket)
> - sounlock(so);
> - pool_put(&socket_pool, so);
> - return (NULL);
> - }
> + if (soreserve(so, head->so_snd.sb_hiwat, head->so_rcv.sb_hiwat))
> + goto error;
sonewconn() has a variable called 'error'. Maybe use some other label.
'fail' and 'bad' are quite frequent in the kernel.
OK visa@
> so->so_snd.sb_wat = head->so_snd.sb_wat;
> so->so_snd.sb_lowat = head->so_snd.sb_lowat;
> so->so_snd.sb_timeo_nsecs = head->so_snd.sb_timeo_nsecs;
> @@ -226,9 +221,6 @@ sonewconn(struct socket *head, int conns
> so->so_rcv.sb_lowat = head->so_rcv.sb_lowat;
> so->so_rcv.sb_timeo_nsecs = head->so_rcv.sb_timeo_nsecs;
>
> - klist_init(&so->so_rcv.sb_klist, &socket_klistops, so);
> - klist_init(&so->so_snd.sb_klist, &socket_klistops, so);
> - sigio_init(&so->so_sigio);
> sigio_copy(&so->so_sigio, &head->so_sigio);
>
> soqinsque(head, so, 0);
> @@ -259,13 +251,7 @@ sonewconn(struct socket *head, int conns
>
> if (error) {
> soqremque(so, 0);
> - if (persocket)
> - sounlock(so);
> - sigio_free(&so->so_sigio);
> - klist_free(&so->so_rcv.sb_klist);
> - klist_free(&so->so_snd.sb_klist);
> - pool_put(&socket_pool, so);
> - return (NULL);
> + goto error;
> }
>
> if (connstatus) {
> @@ -280,6 +266,16 @@ sonewconn(struct socket *head, int conns
> sounlock(so);
>
> return (so);
> +
> +error:
> + if (persocket)
> + sounlock(so);
> + sigio_free(&so->so_sigio);
> + klist_free(&so->so_rcv.sb_klist);
> + klist_free(&so->so_snd.sb_klist);
> + pool_put(&socket_pool, so);
> +
> + return (NULL);
> }
>
> void
> Index: sys/sys/event.h
> ===================================================================
> RCS file: /cvs/src/sys/sys/event.h,v
> retrieving revision 1.67
> diff -u -p -r1.67 event.h
> --- sys/sys/event.h 31 Mar 2022 01:41:22 -0000 1.67
> +++ sys/sys/event.h 31 Jan 2023 18:44:57 -0000
> @@ -286,7 +286,6 @@ struct timespec;
>
> extern const struct filterops sig_filtops;
> extern const struct filterops dead_filtops;
> -extern const struct klistops socket_klistops;
>
> extern void kqpoll_init(unsigned int);
> extern void kqpoll_done(unsigned int);
>
