Re: move pbc list from rawcb to routecb (and later pfkey cb)
On Fri, Jun 30, 2017 at 04:43:06PM +0200, Claudio Jeker wrote:
> Would like to commit this so I can move on.
Just some remarks about variable naming. But please commit as it
is, cleanup on uncommited diffs is difficult.
> -struct pfkeyv2_socket {
> - LIST_ENTRY(pfkeyv2_socket) kcb_list;
> - struct socket *socket;
> +struct keycb {
> + struct rawcbrcb;
> + LIST_ENTRY(keycb) kcb_list;
> int flags;
> uint32_t pid;
> uint32_t registration;/* Increase size if SATYPE_MAX > 31 */
> uint rdomain;
> };
I thinks all these fields should have a kcb_ prefix.
> int
> pfkeyv2_attach(struct socket *so, int proto)
> {
> - struct pfkeyv2_socket *pfkeyv2_socket;
> + struct rawcb *rp;
> + struct keycb *pk;
Could we always name the keycb variable *kp. This would be consistent
to tcpcb *tp, rawcb *rp, routecp *rop.
> int
> -pfkeyv2_detach(struct socket *socket, struct proc *p)
> +pfkeyv2_detach(struct socket *so, struct proc *p)
> {
> - struct pfkeyv2_socket *pp;
> - int error;
> -
> - LIST_FOREACH(pp, &pfkeyv2_sockets, kcb_list)
> - if (pp->socket == socket)
> - break;
> + struct keycb *pk;
Please call it kp.
> int
> -pfkeyv2_sendmessage(void **headers, int mode, struct socket *socket,
> +pfkeyv2_sendmessage(void **headers, int mode, struct socket *so,
> u_int8_t satype, int count, u_int rdomain)
> {
> int i, j, rval;
> void *p, *buffer = NULL;
> struct mbuf *packet;
> - struct pfkeyv2_socket *s;
> + struct keycb *s;
Plaese call it kp.
> int
> -pfkeyv2_send(struct socket *socket, void *message, int len)
> +pfkeyv2_send(struct socket *so, void *message, int len)
> {
> int i, j, s, rval = 0, mode = PFKEYV2_SENDMESSAGE_BROADCAST;
> int delflag = 0;
> @@ -950,7 +942,7 @@ pfkeyv2_send(struct socket *socket, void
> struct radix_node_head *rnh;
> struct radix_node *rn = NULL;
>
> - struct pfkeyv2_socket *pfkeyv2_socket, *so = NULL;
> + struct keycb *pk, *bpk = NULL;
Call it kp, bkp. Useless NULL initialisation.
> struct routecb {
> - struct rawcbrcb;
> - struct timeout timeout;
> - unsigned intmsgfilter;
> - unsigned intflags;
> - u_int rtableid;
> + struct rawcbrcb;
> + LIST_ENTRY(routecb) rcb_list;
> + struct timeout timeout;
> + unsigned intmsgfilter;
> + unsigned intflags;
> + u_int rtableid;
> };
These should have the a rocb_ prefix. Common prefixes per struct
make grepping the source code much easier.
OK bluhm@
Re: move pbc list from rawcb to routecb (and later pfkey cb)
On Tue, May 30, 2017 at 01:59:23PM +0200, Claudio Jeker wrote:
> This is a step I need to do to make progress on the PF_KEY cleanup I'm
> doing. Both PF_ROUTE and PF_KEY need to start to take care of their own
> PCB list and so move the LIST_ENTRY out of rawcb into routecb.
> This allows me to do the same in PF_KEY which will be sent as the next
> diff.
>
Cleaned up and updated diff. Now including both rtsock.c and the pfkey
files. I think I fixed all things reported by mpi@. Especially the socket
handling on create got reordered after better understanding what order is
best. In short as long as the socket() call is not done the file
descriptor for it is invalid and so it is the safest to do all the so
fiddling first and delay the LIST_INSERT to the very end.
Would like to commit this so I can move on.
--
:wq Claudio
Index: net/pfkeyv2.c
===
RCS file: /cvs/src/sys/net/pfkeyv2.c,v
retrieving revision 1.162
diff -u -p -r1.162 pfkeyv2.c
--- net/pfkeyv2.c 26 Jun 2017 09:32:32 - 1.162
+++ net/pfkeyv2.c 30 Jun 2017 14:28:36 -
@@ -131,14 +131,16 @@ extern struct radix_node_head **spd_tabl
struct sockaddr pfkey_addr = { 2, PF_KEY, };
struct domain pfkeydomain;
-struct pfkeyv2_socket {
- LIST_ENTRY(pfkeyv2_socket) kcb_list;
- struct socket *socket;
+struct keycb {
+ struct rawcbrcb;
+ LIST_ENTRY(keycb) kcb_list;
int flags;
uint32_t pid;
uint32_t registration;/* Increase size if SATYPE_MAX > 31 */
uint rdomain;
};
+#define sotokeycb(so) ((struct keycb *)(so)->so_pcb)
+
struct dump_state {
struct sadb_msg *sadb_msg;
@@ -146,8 +148,7 @@ struct dump_state {
};
/* Static globals */
-static LIST_HEAD(, pfkeyv2_socket) pfkeyv2_sockets =
-LIST_HEAD_INITIALIZER(pfkeyv2_sockets);
+static LIST_HEAD(, keycb) pfkeyv2_sockets = LIST_HEAD_INITIALIZER(keycb);
static uint32_t pfkeyv2_seq = 1;
static int nregistered = 0;
static int npromisc = 0;
@@ -160,7 +161,7 @@ int pfkeyv2_usrreq(struct socket *, int,
struct mbuf *, struct proc *);
int pfkeyv2_output(struct mbuf *, struct socket *, struct sockaddr *,
struct mbuf *);
-int pfkey_sendup(struct socket *socket, struct mbuf *packet, int more);
+int pfkey_sendup(struct keycb *, struct mbuf *, int);
/*
* Wrapper around m_devget(); copy data from contiguous buffer to mbuf
@@ -212,72 +213,62 @@ pfkey_init(void)
int
pfkeyv2_attach(struct socket *so, int proto)
{
- struct pfkeyv2_socket *pfkeyv2_socket;
+ struct rawcb *rp;
+ struct keycb *pk;
int error;
if ((so->so_state & SS_PRIV) == 0)
return EACCES;
- if (!(so->so_pcb = malloc(sizeof(struct rawcb),
- M_PCB, M_DONTWAIT | M_ZERO)))
- return (ENOMEM);
-
- error = raw_attach(so, so->so_proto->pr_protocol);
- if (error)
- goto ret;
-
- ((struct rawcb *)so->so_pcb)->rcb_faddr = &pfkey_addr;
+ pk = malloc(sizeof(struct keycb), M_PCB, M_WAITOK | M_ZERO);
+ rp = &pk->rcb;
+ so->so_pcb = rp;
+
+ error = raw_attach(so, proto);
+ if (error) {
+ free(pk, M_PCB, sizeof(struct keycb));
+ return (error);
+ }
- if (!(pfkeyv2_socket = malloc(sizeof(struct pfkeyv2_socket),
- M_PFKEY, M_NOWAIT | M_ZERO)))
- return (ENOMEM);
+ so->so_options |= SO_USELOOPBACK;
+ soisconnected(so);
- LIST_INSERT_HEAD(&pfkeyv2_sockets, pfkeyv2_socket, kcb_list);
- pfkeyv2_socket->socket = so;
- pfkeyv2_socket->pid = curproc->p_p->ps_pid;
+ rp->rcb_faddr = &pfkey_addr;
+ pk->pid = curproc->p_p->ps_pid;
/*
* XXX we should get this from the socket instead but
* XXX rawcb doesn't store the rdomain like inpcb does.
*/
- pfkeyv2_socket->rdomain = rtable_l2(curproc->p_p->ps_rtableid);
+ pk->rdomain = rtable_l2(curproc->p_p->ps_rtableid);
- so->so_options |= SO_USELOOPBACK;
- soisconnected(so);
+ LIST_INSERT_HEAD(&pfkeyv2_sockets, pk, kcb_list);
return (0);
-ret:
- free(so->so_pcb, M_PCB, sizeof(struct rawcb));
- return (error);
}
/*
* Close a PF_KEYv2 socket.
*/
int
-pfkeyv2_detach(struct socket *socket, struct proc *p)
+pfkeyv2_detach(struct socket *so, struct proc *p)
{
- struct pfkeyv2_socket *pp;
- int error;
-
- LIST_FOREACH(pp, &pfkeyv2_sockets, kcb_list)
- if (pp->socket == socket)
- break;
+ struct keycb *pk;
- if (pp) {
- LIST_REMOVE(pp, kcb_list);
+ pk = sotokeycb(so);
+ if (pk == NULL)
+ return ENOTCONN;
- if (pp->flags & PFKEYV2_SOCKETFLAGS_REGISTERED)
- nregistered--;
+ LIST_REMOVE(pk, kcb_list);
- if (pp->flags & PFKEYV2_SOCKETF
Re: move pbc list from rawcb to routecb (and later pfkey cb)
On 05/06/17(Mon) 17:32, Claudio Jeker wrote: > On Mon, Jun 05, 2017 at 11:32:06AM +0200, Martin Pieuchot wrote: > > On 30/05/17(Tue) 13:59, Claudio Jeker wrote: > > > - struct rawcb*rp; > > > struct routecb *rop; > > > int af; > > > int error = 0; > > > > > > - rp = sotorawcb(so); > > > + rop = sotoroutecb(so); > > > + if (rop == NULL) > > > + return ENOTCONN; > > > > Previously raw_usrreq() was returning EINVAL in that case. Does it > > matter? > > > > You should also call m_freem(m), because even if PRU_RCVD and PRU_DETACH > > do not take any argument, we cannot be sure all other code paths cannot > > be reached. That's one of the reasons I'm suggesting we split the PRU > > switches in multiple functions. > > > > What about we make this a KASSERT()? I think it is impossible to get there > with a NULL pointer for the pcb. Fine with me.
Re: move pbc list from rawcb to routecb (and later pfkey cb)
On Mon, Jun 05, 2017 at 11:32:06AM +0200, Martin Pieuchot wrote:
> On 30/05/17(Tue) 13:59, Claudio Jeker wrote:
> > This is a step I need to do to make progress on the PF_KEY cleanup I'm
> > doing. Both PF_ROUTE and PF_KEY need to start to take care of their own
> > PCB list and so move the LIST_ENTRY out of rawcb into routecb.
> > This allows me to do the same in PF_KEY which will be sent as the next
> > diff.
> >
> > --
> > :wq Claudio
> >
> > Index: net/rtsock.c
> > ===
> > RCS file: /cvs/src/sys/net/rtsock.c,v
> > retrieving revision 1.237
> > diff -u -p -r1.237 rtsock.c
> > --- net/rtsock.c19 Apr 2017 15:21:54 - 1.237
> > +++ net/rtsock.c30 May 2017 10:29:25 -
> > @@ -154,45 +157,50 @@ struct route_cb route_cb;
> >
> > #define ROUTE_DESYNC_RESEND_TIMEOUT(hz / 5)/* In hz */
> >
> > +void
> > +route_prinit(void)
> > +{
> > + LIST_INIT(&route_cb.rcb);
> > +}
> > +
> > +
> > int
> > route_usrreq(struct socket *so, int req, struct mbuf *m, struct mbuf *nam,
> > struct mbuf *control, struct proc *p)
> > {
> > - struct rawcb*rp;
> > struct routecb *rop;
> > int af;
> > int error = 0;
> >
> > - rp = sotorawcb(so);
> > + rop = sotoroutecb(so);
> > + if (rop == NULL)
> > + return ENOTCONN;
>
> Previously raw_usrreq() was returning EINVAL in that case. Does it
> matter?
>
> You should also call m_freem(m), because even if PRU_RCVD and PRU_DETACH
> do not take any argument, we cannot be sure all other code paths cannot
> be reached. That's one of the reasons I'm suggesting we split the PRU
> switches in multiple functions.
>
What about we make this a KASSERT()? I think it is impossible to get there
with a NULL pointer for the pcb.
> > @@ -240,10 +248,11 @@ route_attach(struct socket *so, int prot
> > #endif
> > rp->rcb_faddr = &route_src;
> > route_cb.any_count++;
> > + LIST_INSERT_HEAD(&route_cb.rcb, rop, rcb_list);
> > soisconnected(so);
> > so->so_options |= SO_USELOOPBACK;
>
> I'd do the insertion last. It doesn't matter if all operations are
> serialized but it doesn't cost anything and who knows what's coming
> next ;)
Actually that is the reason why I did it there. Doing the socket setup
last to ensure everything is settled before making the socket writeable.
At least the soisconnected() call will move the state of the socket ahead
and therefore other callers may start talking to it. On the other hand
route_input() does a somewhat bad job at figuring out which PCBs are
available.
> Other than that I like this diff very much.
>
--
:wq Claudio
Re: move pbc list from rawcb to routecb (and later pfkey cb)
On 30/05/17(Tue) 13:59, Claudio Jeker wrote:
> This is a step I need to do to make progress on the PF_KEY cleanup I'm
> doing. Both PF_ROUTE and PF_KEY need to start to take care of their own
> PCB list and so move the LIST_ENTRY out of rawcb into routecb.
> This allows me to do the same in PF_KEY which will be sent as the next
> diff.
>
> --
> :wq Claudio
>
> Index: net/rtsock.c
> ===
> RCS file: /cvs/src/sys/net/rtsock.c,v
> retrieving revision 1.237
> diff -u -p -r1.237 rtsock.c
> --- net/rtsock.c 19 Apr 2017 15:21:54 - 1.237
> +++ net/rtsock.c 30 May 2017 10:29:25 -
> @@ -154,45 +157,50 @@ struct route_cb route_cb;
>
> #define ROUTE_DESYNC_RESEND_TIMEOUT (hz / 5)/* In hz */
>
> +void
> +route_prinit(void)
> +{
> + LIST_INIT(&route_cb.rcb);
> +}
> +
> +
> int
> route_usrreq(struct socket *so, int req, struct mbuf *m, struct mbuf *nam,
> struct mbuf *control, struct proc *p)
> {
> - struct rawcb*rp;
> struct routecb *rop;
> int af;
> int error = 0;
>
> - rp = sotorawcb(so);
> + rop = sotoroutecb(so);
> + if (rop == NULL)
> + return ENOTCONN;
Previously raw_usrreq() was returning EINVAL in that case. Does it
matter?
You should also call m_freem(m), because even if PRU_RCVD and PRU_DETACH
do not take any argument, we cannot be sure all other code paths cannot
be reached. That's one of the reasons I'm suggesting we split the PRU
switches in multiple functions.
> @@ -240,10 +248,11 @@ route_attach(struct socket *so, int prot
> #endif
> rp->rcb_faddr = &route_src;
> route_cb.any_count++;
> + LIST_INSERT_HEAD(&route_cb.rcb, rop, rcb_list);
> soisconnected(so);
> so->so_options |= SO_USELOOPBACK;
I'd do the insertion last. It doesn't matter if all operations are
serialized but it doesn't cost anything and who knows what's coming
next ;)
Other than that I like this diff very much.
move pbc list from rawcb to routecb (and later pfkey cb)
This is a step I need to do to make progress on the PF_KEY cleanup I'm
doing. Both PF_ROUTE and PF_KEY need to start to take care of their own
PCB list and so move the LIST_ENTRY out of rawcb into routecb.
This allows me to do the same in PF_KEY which will be sent as the next
diff.
--
:wq Claudio
Index: net/raw_cb.c
===
RCS file: /cvs/src/sys/net/raw_cb.c,v
retrieving revision 1.11
diff -u -p -r1.11 raw_cb.c
--- net/raw_cb.c24 Jan 2017 10:08:30 - 1.11
+++ net/raw_cb.c30 May 2017 08:44:05 -
@@ -46,16 +46,10 @@
/*
* Routines to manage the raw protocol control blocks.
- *
- * TODO:
- * hash lookups by protocol family/protocol + address family
- * take care of unique address problems per AF?
- * redo address binding to allow wildcards
*/
u_long raw_sendspace = RAWSNDQ;
u_long raw_recvspace = RAWRCVQ;
-struct rawcbhead rawcb;
/*
* Allocate a control block and a nominal amount
@@ -72,14 +66,13 @@ raw_attach(struct socket *so, int proto)
* after space has been allocated for the
* rawcb.
*/
- if (rp == 0)
+ if (rp == NULL)
return (ENOBUFS);
if ((error = soreserve(so, raw_sendspace, raw_recvspace)) != 0)
return (error);
rp->rcb_socket = so;
rp->rcb_proto.sp_family = so->so_proto->pr_domain->dom_family;
rp->rcb_proto.sp_protocol = proto;
- LIST_INSERT_HEAD(&rawcb, rp, rcb_list);
return (0);
}
@@ -94,7 +87,6 @@ raw_detach(struct rawcb *rp)
so->so_pcb = 0;
sofree(so);
- LIST_REMOVE(rp, rcb_list);
free((caddr_t)(rp), M_PCB, 0);
}
@@ -104,7 +96,6 @@ raw_detach(struct rawcb *rp)
void
raw_disconnect(struct rawcb *rp)
{
-
if (rp->rcb_socket->so_state & SS_NOFDREF)
raw_detach(rp);
}
Index: net/raw_cb.h
===
RCS file: /cvs/src/sys/net/raw_cb.h,v
retrieving revision 1.11
diff -u -p -r1.11 raw_cb.h
--- net/raw_cb.h23 Jan 2017 16:31:24 - 1.11
+++ net/raw_cb.h30 May 2017 08:44:05 -
@@ -40,7 +40,6 @@
* to tie a socket to the generic raw interface.
*/
struct rawcb {
- LIST_ENTRY(rawcb) rcb_list; /* doubly linked list */
struct socket *rcb_socket; /* back pointer to socket */
struct sockaddr *rcb_faddr;/* destination address */
struct sockaddr *rcb_laddr;/* socket's address */
@@ -54,8 +53,6 @@ struct rawcb {
#defineRAWRCVQ 8192
#ifdef _KERNEL
-
-extern LIST_HEAD(rawcbhead, rawcb) rawcb; /* head of list */
#definesotorawcb(so) ((struct rawcb *)(so)->so_pcb)
int raw_attach(struct socket *, int);
Index: net/raw_usrreq.c
===
RCS file: /cvs/src/sys/net/raw_usrreq.c,v
retrieving revision 1.31
diff -u -p -r1.31 raw_usrreq.c
--- net/raw_usrreq.c13 Mar 2017 20:18:21 - 1.31
+++ net/raw_usrreq.c30 May 2017 08:44:05 -
@@ -45,15 +45,6 @@
#include
#include
-/*
- * Initialize raw connection block q.
- */
-void
-raw_init(void)
-{
-
- LIST_INIT(&rawcb);
-}
int
raw_usrreq(struct socket *so, int req, struct mbuf *m, struct mbuf *nam,
@@ -71,7 +62,7 @@ raw_usrreq(struct socket *so, int req, s
m_freem(m);
return (EOPNOTSUPP);
}
- if (rp == 0) {
+ if (rp == NULL) {
m_freem(m);
return (EINVAL);
}
@@ -81,10 +72,6 @@ raw_usrreq(struct socket *so, int req, s
* Flush data or not depending on the options.
*/
case PRU_DETACH:
- if (rp == 0) {
- error = ENOTCONN;
- break;
- }
raw_detach(rp);
break;
Index: net/rtsock.c
===
RCS file: /cvs/src/sys/net/rtsock.c,v
retrieving revision 1.237
diff -u -p -r1.237 rtsock.c
--- net/rtsock.c19 Apr 2017 15:21:54 - 1.237
+++ net/rtsock.c30 May 2017 10:29:25 -
@@ -98,6 +98,7 @@ struct walkarg {
caddr_t w_where, w_tmem;
};
+void route_prinit(void);
introute_output(struct mbuf *, struct socket *, struct sockaddr *,
struct mbuf *);
introute_ctloutput(int, struct socket *, int, int, struct mbuf *);
@@ -126,19 +127,21 @@ intsysctl_ifnames(struct walkarg *);
int sysctl_rtable_rtstat(void *, size_t *, void *);
struct routecb {
- struct rawcbrcb;
- struct timeout timeout;
- unsigned intmsgfilter;
- unsigned intflags;
- u_int rtableid;
+ struct rawcbrcb;
+ LIST_ENTRY(routecb) rcb_list;
+ struct timeout timeout;
+ unsigned intmsgfilter;
+ u
