On Mon, Feb 06, 2017 at 01:16:45PM +0100, Martin Pieuchot wrote: > On 03/02/17(Fri) 11:02, David Hill wrote: > > On Fri, Feb 03, 2017 at 09:50:40AM +0100, Martin Pieuchot wrote: > > > On 02/02/17(Thu) 12:12, David Hill wrote: > > > > On Thu, Feb 02, 2017 at 09:34:07AM +0100, Martin Pieuchot wrote: > > > > > On 01/02/17(Wed) 19:27, David Hill wrote: > > > > > > Hello - > > > > > > > > > > > > This diff makes sosetopt responsible for m_free which is much > > > > > > simpler. > > > > > > Requested by bluhm@ > > > > > > > > > > I'd suggest to move the m_free(9) calls to sys_setsockopt(). This > > > > > simplifies the existing code even more and will make it easier to use > > > > > the stack for this temporary storage. > > > > > > > > > > > > > New diff with mpi@'s suggestion. > > > > > > You forgot NFS and BFD that should now call m_free(9) after sosetopt(9). > > > > > > > Indeed! Now with BFD and NFS... > > You're introducing a use after-free in ip_pcbopts(). You need to > allocate/copy the mbuf there. > > I must say I'm a bit afraid of this change because the amount of code it > touches. There might be another use after free somewhere that I missed. > > Maybe we should first split our huge *ctloutput functions. > > One easy move is to split setopt/getopt. > > Then introduce more per-protocol functions instead of having everything > in ip{,6}_ctloutput(). > > For example move all the IPSEC craziness out of these functions. Same > thing with ICMP6... This might sound superfluous but it will help > if/when we decide to have a fine graining for different subsystems. > > I'd also suggest to change the 'struct protosw' declaration to use C99 > initializer. So we can have: > > .pr_ctloutput = ipsec_ctloutput > > This would allow us to grep for "pr_ctloutput" (or pr_setopt) and know > directly which functions to review. >
If you are OK with first splitting each *ctloutput into *getopt/*setopt, I will send each diff individually to make review easier. Here is the first split, route_ctloutput. Index: net/rtsock.c =================================================================== RCS file: /cvs/src/sys/net/rtsock.c,v retrieving revision 1.222 diff -u -p -r1.222 rtsock.c --- net/rtsock.c 1 Feb 2017 20:59:47 -0000 1.222 +++ net/rtsock.c 21 Feb 2017 15:52:07 -0000 @@ -98,6 +98,8 @@ struct walkarg { caddr_t w_where, w_tmem; }; +int route_getopt(struct socket *, int, int, struct mbuf *); +int route_setopt(struct socket *, int, int, struct mbuf *); int route_ctloutput(int, struct socket *, int, int, struct mbuf *); void route_input(struct mbuf *m0, sa_family_t); int route_arp_conflict(struct rtentry *, struct rt_addrinfo *); @@ -233,62 +235,86 @@ route_usrreq(struct socket *so, int req, } int -route_ctloutput(int op, struct socket *so, int level, int optname, - struct mbuf *m) +route_getopt(struct socket *so, int level, int optname, struct mbuf *m) +{ + struct routecb *rop = sotoroutecb(so); + int error = 0; + + if (level != AF_ROUTE) + return EINVAL; + + switch (optname) { + case ROUTE_MSGFILTER: + m->m_len = sizeof(unsigned int); + *mtod(m, unsigned int *) = rop->msgfilter; + break; + case ROUTE_TABLEFILTER: + m->m_len = sizeof(unsigned int); + *mtod(m, unsigned int *) = rop->rtableid; + break; + default: + error = ENOPROTOOPT; + break; + } + return error; +} + +int +route_setopt(struct socket *so, int level, int optname, struct mbuf *m) { struct routecb *rop = sotoroutecb(so); int error = 0; unsigned int tid; if (level != AF_ROUTE) { - error = EINVAL; - if (op == PRCO_SETOPT && m) - m_free(m); - return (error); + m_free(m); + return EINVAL; } - switch (op) { - case PRCO_SETOPT: - switch (optname) { - case ROUTE_MSGFILTER: - if (m == NULL || m->m_len != sizeof(unsigned int)) - error = EINVAL; - else - rop->msgfilter = *mtod(m, unsigned int *); - break; - case ROUTE_TABLEFILTER: - if (m == NULL || m->m_len != sizeof(unsigned int)) { - error = EINVAL; - break; - } - tid = *mtod(m, unsigned int *); - if (tid != RTABLE_ANY && !rtable_exists(tid)) - error = ENOENT; - else - rop->rtableid = tid; - break; - default: - error = ENOPROTOOPT; + switch (optname) { + case ROUTE_MSGFILTER: + if (m == NULL || m->m_len != sizeof(unsigned int)) + error = EINVAL; + else + rop->msgfilter = *mtod(m, unsigned int *); + break; + case ROUTE_TABLEFILTER: + if (m == NULL || m->m_len != sizeof(unsigned int)) { + error = EINVAL; break; } - m_free(m); + tid = *mtod(m, unsigned int *); + if (tid != RTABLE_ANY && !rtable_exists(tid)) + error = ENOENT; + else + rop->rtableid = tid; + break; + default: + error = ENOPROTOOPT; break; + } + m_free(m); + return error; +} + +int +route_ctloutput(int op, struct socket *so, int level, int optname, + struct mbuf *m) +{ + int error; + + switch (op) { case PRCO_GETOPT: - switch (optname) { - case ROUTE_MSGFILTER: - m->m_len = sizeof(unsigned int); - *mtod(m, unsigned int *) = rop->msgfilter; - break; - case ROUTE_TABLEFILTER: - m->m_len = sizeof(unsigned int); - *mtod(m, unsigned int *) = rop->rtableid; - break; - default: - error = ENOPROTOOPT; - break; - } + error = route_getopt(so, level, optname, m); + break; + case PRCO_SETOPT: + error = route_setopt(so, level, optname, m); + break; + default: + error = EINVAL; + break; } - return (error); + return error; } void