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
