On Thu, Apr 27, 2023 at 01:55:33PM +0300, Vitaliy Makkoveev wrote:
> Now only direct netlock used for inet sockets protection. The unlocked
> access to all other sockets is safe, but we could lost consistency for a
> little. Since the solock() used for sockets protection, make locking
> path common and use it. Make it shared because this is read-only access
> to sockets data. For same reason use shared netlock while performing
> inpcb tables foreach walkthrough.
This is still wrong. fill_file() is not allowed to sleep. So you can not
use a rwlock in here. fill_file is called inside a allprocess
LIST_FOREACH loop and sleeping there will blow up.
Maybe it is enough to grab a shared NETLOCK around the LIST_FOREACH() to
ensure that we don't sleep inside.
Please fix this properly. Right now running fstat is like playing russian
roulette.
> Index: sys/kern/kern_sysctl.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/kern_sysctl.c,v
> retrieving revision 1.411
> diff -u -p -r1.411 kern_sysctl.c
> --- sys/kern/kern_sysctl.c 22 Jan 2023 12:05:44 -0000 1.411
> +++ sys/kern/kern_sysctl.c 27 Apr 2023 10:18:01 -0000
> @@ -1173,14 +1173,11 @@ fill_file(struct kinfo_file *kf, struct
>
> if (so == NULL) {
> so = (struct socket *)fp->f_data;
> + solock_shared(so);
> + locked = 1;
> + } else {
> /* if so is passed as parameter it is already locked */
> - switch (so->so_proto->pr_domain->dom_family) {
> - case AF_INET:
> - case AF_INET6:
> - NET_LOCK();
> - locked = 1;
> - break;
> - }
> + soassertlocked(so);
> }
>
> kf->so_type = so->so_type;
> @@ -1203,14 +1200,13 @@ fill_file(struct kinfo_file *kf, struct
> kf->so_splicelen = -1;
> if (so->so_pcb == NULL) {
> if (locked)
> - NET_UNLOCK();
> + sounlock_shared(so);
> break;
> }
> switch (kf->so_family) {
> case AF_INET: {
> struct inpcb *inpcb = so->so_pcb;
>
> - NET_ASSERT_LOCKED();
> if (show_pointers)
> kf->inp_ppcb = PTRTOINT64(inpcb->inp_ppcb);
> kf->inp_lport = inpcb->inp_lport;
> @@ -1232,7 +1228,6 @@ fill_file(struct kinfo_file *kf, struct
> case AF_INET6: {
> struct inpcb *inpcb = so->so_pcb;
>
> - NET_ASSERT_LOCKED();
> if (show_pointers)
> kf->inp_ppcb = PTRTOINT64(inpcb->inp_ppcb);
> kf->inp_lport = inpcb->inp_lport;
> @@ -1279,7 +1274,7 @@ fill_file(struct kinfo_file *kf, struct
> }
> }
> if (locked)
> - NET_UNLOCK();
> + sounlock_shared(so);
> break;
> }
>
> @@ -1375,7 +1370,7 @@ sysctl_file(int *name, u_int namelen, ch
> if (arg == DTYPE_SOCKET) {
> struct inpcb *inp;
>
> - NET_LOCK();
> + NET_LOCK_SHARED();
> mtx_enter(&tcbtable.inpt_mtx);
> TAILQ_FOREACH(inp, &tcbtable.inpt_queue, inp_queue)
> FILLSO(inp->inp_socket);
> @@ -1395,7 +1390,7 @@ sysctl_file(int *name, u_int namelen, ch
> FILLSO(inp->inp_socket);
> mtx_leave(&rawin6pcbtable.inpt_mtx);
> #endif
> - NET_UNLOCK();
> + NET_UNLOCK_SHARED();
> }
> fp = NULL;
> while ((fp = fd_iterfile(fp, p)) != NULL) {
>
--
:wq Claudio
