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.
> Index: kern/uipc_socket.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/uipc_socket.c,v
> retrieving revision 1.176
> diff -u -p -r1.176 uipc_socket.c
> --- kern/uipc_socket.c 1 Feb 2017 20:59:47 -0000 1.176
> +++ kern/uipc_socket.c 3 Feb 2017 16:00:26 -0000
> @@ -1551,16 +1551,15 @@ sowwakeup(struct socket *so)
> }
>
> int
> -sosetopt(struct socket *so, int level, int optname, struct mbuf *m0)
> +sosetopt(struct socket *so, int level, int optname, struct mbuf *m)
> {
> int s, error = 0;
> - struct mbuf *m = m0;
>
> if (level != SOL_SOCKET) {
> if (so->so_proto && so->so_proto->pr_ctloutput) {
> NET_LOCK(s);
> error = (*so->so_proto->pr_ctloutput)(PRCO_SETOPT, so,
> - level, optname, m0);
> + level, optname, m);
> NET_UNLOCK(s);
> return (error);
> }
> @@ -1707,7 +1706,7 @@ sosetopt(struct socket *so, int level, i
> level = dom->dom_protosw->pr_protocol;
> NET_LOCK(s);
> error = (*so->so_proto->pr_ctloutput)
> - (PRCO_SETOPT, so, level, optname, m0);
> + (PRCO_SETOPT, so, level, optname, m);
> NET_UNLOCK(s);
> return (error);
> }
> @@ -1739,14 +1738,11 @@ sosetopt(struct socket *so, int level, i
> if (error == 0 && so->so_proto && so->so_proto->pr_ctloutput) {
> NET_LOCK(s);
> (*so->so_proto->pr_ctloutput)(PRCO_SETOPT, so,
> - level, optname, m0);
> + level, optname, m);
> NET_UNLOCK(s);
> - m = NULL; /* freed by protocol */
> }
> }
> bad:
> - if (m)
> - (void) m_free(m);
> return (error);
> }
>
> Index: kern/uipc_syscalls.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/uipc_syscalls.c,v
> retrieving revision 1.148
> diff -u -p -r1.148 uipc_syscalls.c
> --- kern/uipc_syscalls.c 26 Jan 2017 01:58:00 -0000 1.148
> +++ kern/uipc_syscalls.c 3 Feb 2017 16:00:26 -0000
> @@ -962,19 +962,13 @@ sys_setsockopt(struct proc *p, void *v,
> goto bad;
> }
> }
> - if (m == NULL) {
> - error = ENOBUFS;
> - goto bad;
> - }
> error = copyin(SCARG(uap, val), mtod(m, caddr_t),
> SCARG(uap, valsize));
> - if (error) {
> + if (error)
> goto bad;
> - }
> m->m_len = SCARG(uap, valsize);
> }
> error = sosetopt(fp->f_data, SCARG(uap, level), SCARG(uap, name), m);
> - m = NULL;
> bad:
> m_freem(m);
> FRELE(fp, p);
> Index: net/bfd.c
> ===================================================================
> RCS file: /cvs/src/sys/net/bfd.c,v
> retrieving revision 1.58
> diff -u -p -r1.58 bfd.c
> --- net/bfd.c 24 Jan 2017 10:08:30 -0000 1.58
> +++ net/bfd.c 3 Feb 2017 16:00:26 -0000
> @@ -418,7 +418,7 @@ bfd_listener(struct bfd_config *bfd, uns
> struct sockaddr_in *sin;
> struct sockaddr_in6 *sin6;
> struct socket *so;
> - struct mbuf *m = NULL, *mopt = NULL;
> + struct mbuf *m = NULL, *mopt;
> int *ip, error;
>
> /* sa_family and sa_len must be equal */
> @@ -437,6 +437,7 @@ bfd_listener(struct bfd_config *bfd, uns
> ip = mtod(mopt, int *);
> *ip = MAXTTL;
> error = sosetopt(so, IPPROTO_IP, IP_MINTTL, mopt);
> + m_free(mopt);
> if (error) {
> printf("%s: sosetopt error %d\n",
> __func__, error);
> @@ -487,7 +488,7 @@ bfd_sender(struct bfd_config *bfd, unsig
> struct socket *so;
> struct rtentry *rt = bfd->bc_rt;
> struct proc *p = curproc;
> - struct mbuf *m = NULL, *mopt = NULL;
> + struct mbuf *m = NULL, *mopt;
> struct sockaddr *src = rt->rt_ifa->ifa_addr;
> struct sockaddr *dst = rt_key(rt);
> struct sockaddr *sa;
> @@ -509,6 +510,7 @@ bfd_sender(struct bfd_config *bfd, unsig
> ip = mtod(mopt, int *);
> *ip = IP_PORTRANGE_HIGH;
> error = sosetopt(so, IPPROTO_IP, IP_PORTRANGE, mopt);
> + m_free(mopt);
> if (error) {
> printf("%s: sosetopt error %d\n",
> __func__, error);
> @@ -520,6 +522,7 @@ bfd_sender(struct bfd_config *bfd, unsig
> ip = mtod(mopt, int *);
> *ip = MAXTTL;
> error = sosetopt(so, IPPROTO_IP, IP_TTL, mopt);
> + m_free(mopt);
> if (error) {
> printf("%s: sosetopt error %d\n",
> __func__, error);
> @@ -531,6 +534,7 @@ bfd_sender(struct bfd_config *bfd, unsig
> ip = mtod(mopt, int *);
> *ip = IPTOS_PREC_INTERNETCONTROL;
> error = sosetopt(so, IPPROTO_IP, IP_TOS, mopt);
> + m_free(mopt);
> if (error) {
> printf("%s: sosetopt error %d\n",
> __func__, error);
> 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 3 Feb 2017 16:00:26 -0000
> @@ -240,12 +240,8 @@ route_ctloutput(int op, struct socket *s
> int error = 0;
> unsigned int tid;
>
> - if (level != AF_ROUTE) {
> - error = EINVAL;
> - if (op == PRCO_SETOPT && m)
> - m_free(m);
> - return (error);
> - }
> + if (level != AF_ROUTE)
> + return EINVAL;
>
> switch (op) {
> case PRCO_SETOPT:
> @@ -271,7 +267,6 @@ route_ctloutput(int op, struct socket *s
> error = ENOPROTOOPT;
> break;
> }
> - m_free(m);
> break;
> case PRCO_GETOPT:
> switch (optname) {
> Index: netinet/ip_mroute.c
> ===================================================================
> RCS file: /cvs/src/sys/netinet/ip_mroute.c,v
> retrieving revision 1.108
> diff -u -p -r1.108 ip_mroute.c
> --- netinet/ip_mroute.c 1 Feb 2017 20:59:47 -0000 1.108
> +++ netinet/ip_mroute.c 3 Feb 2017 16:00:26 -0000
> @@ -209,7 +209,6 @@ ip_mrouter_set(struct socket *so, int op
> break;
> }
>
> - m_free(m);
> return (error);
> }
>
> Index: netinet/ip_output.c
> ===================================================================
> RCS file: /cvs/src/sys/netinet/ip_output.c,v
> retrieving revision 1.335
> diff -u -p -r1.335 ip_output.c
> --- netinet/ip_output.c 1 Feb 2017 20:59:47 -0000 1.335
> +++ netinet/ip_output.c 3 Feb 2017 16:00:26 -0000
> @@ -853,11 +853,10 @@ ip_ctloutput(int op, struct socket *so,
> int error = 0;
> u_int rtid = 0;
>
> - if (level != IPPROTO_IP) {
> - error = EINVAL;
> - if (op == PRCO_SETOPT)
> - (void) m_free(m);
> - } else switch (op) {
> + if (level != IPPROTO_IP)
> + return EINVAL;
> +
> + switch (op) {
> case PRCO_SETOPT:
> switch (optname) {
> case IP_OPTIONS:
> @@ -1073,7 +1072,6 @@ ip_ctloutput(int op, struct socket *so,
> error = ENOPROTOOPT;
> break;
> }
> - m_free(m);
> break;
>
> case PRCO_GETOPT:
> @@ -1235,12 +1233,11 @@ ip_pcbopts(struct mbuf **pcbopt, struct
>
> /* turn off any old options */
> m_free(*pcbopt);
> - *pcbopt = 0;
> + *pcbopt = NULL;
> if (m == NULL || m->m_len == 0) {
> /*
> * Only turning off any previous options.
> */
> - m_free(m);
> return (0);
> }
>
> @@ -1316,7 +1313,6 @@ ip_pcbopts(struct mbuf **pcbopt, struct
> return (0);
>
> bad:
> - (void)m_free(m);
> return (EINVAL);
> }
>
> Index: netinet/raw_ip.c
> ===================================================================
> RCS file: /cvs/src/sys/netinet/raw_ip.c,v
> retrieving revision 1.95
> diff -u -p -r1.95 raw_ip.c
> --- netinet/raw_ip.c 1 Feb 2017 20:59:47 -0000 1.95
> +++ netinet/raw_ip.c 3 Feb 2017 16:00:26 -0000
> @@ -305,11 +305,8 @@ rip_ctloutput(int op, struct socket *so,
> int error = 0;
> int dir;
>
> - if (level != IPPROTO_IP) {
> - if (op == PRCO_SETOPT)
> - (void) m_free(m);
> - return (EINVAL);
> - }
> + if (level != IPPROTO_IP)
> + return EINVAL;
>
> switch (optname) {
>
> @@ -322,7 +319,6 @@ rip_ctloutput(int op, struct socket *so,
> inp->inp_flags |= INP_HDRINCL;
> else
> inp->inp_flags &= ~INP_HDRINCL;
> - m_free(m);
> } else {
> m->m_len = sizeof(int);
> *mtod(m, int *) = inp->inp_flags & INP_HDRINCL;
> @@ -344,7 +340,6 @@ rip_ctloutput(int op, struct socket *so,
> inp->inp_divertfl = dir;
> else
> error = EINVAL;
> -
> break;
>
> case PRCO_GETOPT:
> @@ -357,8 +352,6 @@ rip_ctloutput(int op, struct socket *so,
> break;
> }
>
> - if (op == PRCO_SETOPT)
> - (void)m_free(m);
> return (error);
>
> case MRT_INIT:
> @@ -385,8 +378,6 @@ rip_ctloutput(int op, struct socket *so,
> }
> return (error);
> #else
> - if (op == PRCO_SETOPT)
> - m_free(m);
> return (EOPNOTSUPP);
> #endif
> }
> Index: netinet/tcp_usrreq.c
> ===================================================================
> RCS file: /cvs/src/sys/netinet/tcp_usrreq.c,v
> retrieving revision 1.143
> diff -u -p -r1.143 tcp_usrreq.c
> --- netinet/tcp_usrreq.c 1 Feb 2017 20:59:47 -0000 1.143
> +++ netinet/tcp_usrreq.c 3 Feb 2017 16:00:27 -0000
> @@ -457,11 +457,9 @@ tcp_ctloutput(int op, struct socket *so,
> int i;
>
> inp = sotoinpcb(so);
> - if (inp == NULL) {
> - if (op == PRCO_SETOPT)
> - (void) m_free(m);
> - return (ECONNRESET);
> - }
> + if (inp == NULL)
> + return ECONNRESET;
> +
> if (level != IPPROTO_TCP) {
> switch (so->so_proto->pr_domain->dom_family) {
> #ifdef INET6
> @@ -481,10 +479,8 @@ tcp_ctloutput(int op, struct socket *so,
> tp = intotcpcb(inp);
>
> switch (op) {
> -
> case PRCO_SETOPT:
> switch (optname) {
> -
> case TCP_NODELAY:
> if (m == NULL || m->m_len < sizeof (int))
> error = EINVAL;
> @@ -567,7 +563,6 @@ tcp_ctloutput(int op, struct socket *so,
> error = ENOPROTOOPT;
> break;
> }
> - m_free(m);
> break;
>
> case PRCO_GETOPT:
> Index: netinet6/icmp6.c
> ===================================================================
> RCS file: /cvs/src/sys/netinet6/icmp6.c,v
> retrieving revision 1.198
> diff -u -p -r1.198 icmp6.c
> --- netinet6/icmp6.c 1 Feb 2017 20:59:47 -0000 1.198
> +++ netinet6/icmp6.c 3 Feb 2017 16:00:27 -0000
> @@ -1807,11 +1807,8 @@ icmp6_ctloutput(int op, struct socket *s
> int error = 0;
> struct inpcb *in6p = sotoinpcb(so);
>
> - if (level != IPPROTO_ICMPV6) {
> - if (op == PRCO_SETOPT)
> - (void)m_free(m);
> + if (level != IPPROTO_ICMPV6)
> return EINVAL;
> - }
>
> switch (op) {
> case PRCO_SETOPT:
> @@ -1839,7 +1836,6 @@ icmp6_ctloutput(int op, struct socket *s
> error = ENOPROTOOPT;
> break;
> }
> - m_freem(m);
> break;
>
> case PRCO_GETOPT:
> Index: netinet6/ip6_output.c
> ===================================================================
> RCS file: /cvs/src/sys/netinet6/ip6_output.c,v
> retrieving revision 1.223
> diff -u -p -r1.223 ip6_output.c
> --- netinet6/ip6_output.c 1 Feb 2017 20:59:47 -0000 1.223
> +++ netinet6/ip6_output.c 3 Feb 2017 16:00:27 -0000
> @@ -1390,7 +1390,6 @@ do { \
> error = ENOPROTOOPT;
> break;
> }
> - m_free(m);
> break;
>
> case PRCO_GETOPT:
> @@ -1591,11 +1590,8 @@ do { \
> }
> break;
> }
> - } else {
> + } else
> error = EINVAL;
> - if (op == PRCO_SETOPT)
> - (void)m_free(m);
> - }
> return (error);
> }
>
> @@ -1607,11 +1603,8 @@ ip6_raw_ctloutput(int op, struct socket
> const int icmp6off = offsetof(struct icmp6_hdr, icmp6_cksum);
> struct inpcb *inp = sotoinpcb(so);
>
> - if (level != IPPROTO_IPV6) {
> - if (op == PRCO_SETOPT)
> - (void)m_free(m);
> + if (level != IPPROTO_IPV6)
> return (EINVAL);
> - }
>
> switch (optname) {
> case IPV6_CHECKSUM:
> @@ -1660,9 +1653,6 @@ ip6_raw_ctloutput(int op, struct socket
> error = ENOPROTOOPT;
> break;
> }
> -
> - if (op == PRCO_SETOPT)
> - (void)m_free(m);
>
> return (error);
> }
> Index: netinet6/raw_ip6.c
> ===================================================================
> RCS file: /cvs/src/sys/netinet6/raw_ip6.c,v
> retrieving revision 1.104
> diff -u -p -r1.104 raw_ip6.c
> --- netinet6/raw_ip6.c 1 Feb 2017 20:59:47 -0000 1.104
> +++ netinet6/raw_ip6.c 3 Feb 2017 16:00:27 -0000
> @@ -511,8 +511,6 @@ rip6_ctloutput(int op, struct socket *so
> break;
> }
>
> - if (op == PRCO_SETOPT)
> - (void)m_free(m);
> return (error);
>
> #ifdef MROUTING
> @@ -522,10 +520,9 @@ rip6_ctloutput(int op, struct socket *so
> case MRT6_DEL_MIF:
> case MRT6_ADD_MFC:
> case MRT6_DEL_MFC:
> - if (op == PRCO_SETOPT) {
> + if (op == PRCO_SETOPT)
> error = ip6_mrouter_set(optname, so, m);
> - m_free(m);
> - } else if (op == PRCO_GETOPT)
> + else if (op == PRCO_GETOPT)
> error = ip6_mrouter_get(optname, so, m);
> else
> error = EINVAL;
> @@ -545,8 +542,6 @@ rip6_ctloutput(int op, struct socket *so
> return (icmp6_ctloutput(op, so, level, optname, m));
>
> default:
> - if (op == PRCO_SETOPT)
> - m_free(m);
> return EINVAL;
> }
> }
> Index: nfs/krpc_subr.c
> ===================================================================
> RCS file: /cvs/src/sys/nfs/krpc_subr.c,v
> retrieving revision 1.29
> diff -u -p -r1.29 krpc_subr.c
> --- nfs/krpc_subr.c 24 Aug 2015 14:00:29 -0000 1.29
> +++ nfs/krpc_subr.c 3 Feb 2017 16:00:27 -0000
> @@ -239,7 +239,9 @@ krpc_call(struct sockaddr_in *sa, u_int
> tv.tv_usec = 0;
> memcpy(mtod(m, struct timeval *), &tv, sizeof tv);
> m->m_len = sizeof(tv);
> - if ((error = sosetopt(so, SOL_SOCKET, SO_RCVTIMEO, m)))
> + error = sosetopt(so, SOL_SOCKET, SO_RCVTIMEO, m);
> + m_free(m);
> + if (error)
> goto out;
>
> /*
> @@ -251,7 +253,9 @@ krpc_call(struct sockaddr_in *sa, u_int
> on = mtod(m, int32_t *);
> m->m_len = sizeof(*on);
> *on = 1;
> - if ((error = sosetopt(so, SOL_SOCKET, SO_BROADCAST, m)))
> + error = sosetopt(so, SOL_SOCKET, SO_BROADCAST, m);
> + m_free(m);
> + if (error)
> goto out;
> }
>
> @@ -265,6 +269,7 @@ krpc_call(struct sockaddr_in *sa, u_int
> ip = mtod(mopt, int *);
> *ip = IP_PORTRANGE_LOW;
> error = sosetopt(so, IPPROTO_IP, IP_PORTRANGE, mopt);
> + m_free(mopt);
> if (error)
> goto out;
>
> @@ -287,6 +292,7 @@ krpc_call(struct sockaddr_in *sa, u_int
> ip = mtod(mopt, int *);
> *ip = IP_PORTRANGE_DEFAULT;
> error = sosetopt(so, IPPROTO_IP, IP_PORTRANGE, mopt);
> + m_free(mopt);
> if (error)
> goto out;
>
> Index: nfs/nfs_socket.c
> ===================================================================
> RCS file: /cvs/src/sys/nfs/nfs_socket.c,v
> retrieving revision 1.112
> diff -u -p -r1.112 nfs_socket.c
> --- nfs/nfs_socket.c 19 Dec 2016 08:36:50 -0000 1.112
> +++ nfs/nfs_socket.c 3 Feb 2017 16:00:27 -0000
> @@ -253,6 +253,7 @@ nfs_connect(struct nfsmount *nmp, struct
> ip = mtod(mopt, int *);
> *ip = IP_PORTRANGE_LOW;
> error = sosetopt(so, IPPROTO_IP, IP_PORTRANGE, mopt);
> + m_free(mopt);
> if (error)
> goto bad;
>
> @@ -273,6 +274,7 @@ nfs_connect(struct nfsmount *nmp, struct
> ip = mtod(mopt, int *);
> *ip = IP_PORTRANGE_DEFAULT;
> error = sosetopt(so, IPPROTO_IP, IP_PORTRANGE, mopt);
> + m_free(mopt);
> if (error)
> goto bad;
> }
> @@ -341,12 +343,14 @@ nfs_connect(struct nfsmount *nmp, struct
> *mtod(m, int32_t *) = 1;
> m->m_len = sizeof(int32_t);
> sosetopt(so, SOL_SOCKET, SO_KEEPALIVE, m);
> + m_free(m);
> }
> if (so->so_proto->pr_protocol == IPPROTO_TCP) {
> MGET(m, M_WAIT, MT_SOOPTS);
> *mtod(m, int32_t *) = 1;
> m->m_len = sizeof(int32_t);
> sosetopt(so, IPPROTO_TCP, TCP_NODELAY, m);
> + m_free(m);
> }
> sndreserve = (nmp->nm_wsize + NFS_MAXPKTHDR +
> sizeof (u_int32_t)) * 2;
> Index: nfs/nfs_syscalls.c
> ===================================================================
> RCS file: /cvs/src/sys/nfs/nfs_syscalls.c,v
> retrieving revision 1.106
> diff -u -p -r1.106 nfs_syscalls.c
> --- nfs/nfs_syscalls.c 15 Sep 2016 02:00:18 -0000 1.106
> +++ nfs/nfs_syscalls.c 3 Feb 2017 16:00:27 -0000
> @@ -255,6 +255,7 @@ nfssvc_addsock(struct file *fp, struct m
> *mtod(m, int32_t *) = 1;
> m->m_len = sizeof(int32_t);
> sosetopt(so, SOL_SOCKET, SO_KEEPALIVE, m);
> + m_free(m);
> }
> if (so->so_proto->pr_domain->dom_family == AF_INET &&
> so->so_proto->pr_protocol == IPPROTO_TCP) {
> @@ -262,6 +263,7 @@ nfssvc_addsock(struct file *fp, struct m
> *mtod(m, int32_t *) = 1;
> m->m_len = sizeof(int32_t);
> sosetopt(so, IPPROTO_TCP, TCP_NODELAY, m);
> + m_free(m);
> }
> so->so_rcv.sb_flags &= ~SB_NOINTR;
> so->so_rcv.sb_timeo = 0;
> @@ -270,8 +272,7 @@ nfssvc_addsock(struct file *fp, struct m
> if (tslp)
> slp = tslp;
> else {
> - slp = malloc(sizeof(*slp), M_NFSSVC,
> - M_WAITOK|M_ZERO);
> + slp = malloc(sizeof(*slp), M_NFSSVC, M_WAITOK|M_ZERO);
> TAILQ_INSERT_TAIL(&nfssvc_sockhead, slp, ns_chain);
> }
> slp->ns_so = so;
>
