DMAC-driver, bus_dma & ofw_dmac
Hi,
i've been slowly looking at bringing dma for some of the lesser sunxis,
and have reached the point where i'm unsure what to do next beyond
testing the functionality w/spreading SoC-specific hacks "for testing"..
now even if i will manage to shove it partly under _bus_dma_tag, it will
not be usable w/o a hack/special-casing either, so that lead to looking
at implementing ofw_dmac, which i can't finish because i can't decide on
it's interface, i mean there's too many completely new pieces to me in
the puzzle after all, o and the naming; ofw_dma or ofw_dmac? can't pick.
DT-binding properties for the generic dma-controller did seem simple,
and in my case the Allwinner A10 DMA Controller follows those, but w/o
knowing better continuing would likely be waste of my time:)
currently i have something like this in sys/dev/ofw/ofw_dmac.h:
struct dma_controller {
int dc_node;
/*
* Pointer to struct x_bus_space_tag, containing
* another _cookie to struct x_softc of our parent.
*/
void*dc_cookie;
/* claim/disown channel|w/e controller specific */
void(*dc_claim)(void *. uint32_t *, int);
/* dc_cookie, &cells[1], disown:0|claim:1 */
LIST_ENTRY(dma_controller) dc_list;
uint32_t dc_phandle;
uint32_t dc_cells;
#ifdef notyet
uint32_t dc_channels;
uint32_t dc_requests;
#endif
};
voiddmac_register(struct dma_controller *);
/*
* does return the number of entries in the "dmas" property,
* and the bus_dma_tag_t for client to use.
*/
int dmac_register_client(int, bus_dma_tag_t *);
voiddmac_claim(int, const char *);
voiddmac_claim_idx(int, int);
voiddmac_disown(int, const char *);
voiddmac_disown_idx(int, int);
for clients that'd be quite simple to use to get more specific
bus_dma_tag than given in fdt_attach_args, so ie. in drivers
used across different dma controllers, im hoping above might
work w/clients calling _register_client unconditionally? or?
any input more than welcome:)
-Artturi
Re: ftp: use mono clock for transfer estimates, stats
2 week bump.
--
Scott Cheloha
> On Aug 19, 2017, at 9:40 AM, Scott Cheloha wrote:
>
> Hi,
>
> Same deal here as in dd(1). We're displaying an elapsed time
> so we want a monotonic clock.
>
> Because everything printed is derived from elapsed, this one
> was relatively simple.
>
> Light testing shows this fixes the corner case where the host
> time is wound backward mid-transfer.
>
> Feedback?
>
> --
> Scott Cheloha
>
> Index: usr.bin/ftp/util.c
> ===
> RCS file: /cvs/src/usr.bin/ftp/util.c,v
> retrieving revision 1.84
> diff -u -p -r1.84 util.c
> --- usr.bin/ftp/util.c21 Jan 2017 08:33:07 - 1.84
> +++ usr.bin/ftp/util.c19 Aug 2017 14:24:44 -
> @@ -744,7 +744,7 @@ updateprogressmeter(int signo)
> * with flag = 0
> * - After the transfer, call with flag = 1
> */
> -static struct timeval start;
> +static struct timespec start;
>
> char *action;
>
> @@ -757,21 +757,21 @@ progressmeter(int flag, const char *file
>*/
> static const char prefixes[] = " KMGTP";
>
> - static struct timeval lastupdate;
> + static struct timespec lastupdate;
> static off_t lastsize;
> static char *title = NULL;
> - struct timeval now, td, wait;
> + struct timespec now, td, wait;
> off_t cursize, abbrevsize;
> double elapsed;
> int ratio, barlength, i, remaining, overhead = 30;
> char buf[512];
>
> if (flag == -1) {
> - (void)gettimeofday(&start, NULL);
> + clock_gettime(CLOCK_MONOTONIC, &start);
> lastupdate = start;
> lastsize = restart_point;
> }
> - (void)gettimeofday(&now, NULL);
> + clock_gettime(CLOCK_MONOTONIC, &now);
> if (!progress || filesize < 0)
> return;
> cursize = bytes + restart_point;
> @@ -849,19 +849,19 @@ progressmeter(int flag, const char *file
> " %5lld %c%c ", (long long)abbrevsize, prefixes[i],
> prefixes[i] == ' ' ? ' ' : 'B');
>
> - timersub(&now, &lastupdate, &wait);
> + timespecsub(&now, &lastupdate, &wait);
> if (cursize > lastsize) {
> lastupdate = now;
> lastsize = cursize;
> if (wait.tv_sec >= STALLTIME) { /* fudge out stalled time */
> start.tv_sec += wait.tv_sec;
> - start.tv_usec += wait.tv_usec;
> + start.tv_nsec += wait.tv_nsec;
> }
> wait.tv_sec = 0;
> }
>
> - timersub(&now, &start, &td);
> - elapsed = td.tv_sec + (td.tv_usec / 100.0);
> + timespecsub(&now, &start, &td);
> + elapsed = td.tv_sec + (td.tv_nsec / 10.0);
>
> if (flag == 1) {
> i = (int)elapsed / 3600;
> @@ -919,7 +919,7 @@ progressmeter(int flag, const char *file
> void
> ptransfer(int siginfo)
> {
> - struct timeval now, td;
> + struct timespec now, td;
> double elapsed;
> off_t bs;
> int meg, remaining, hh;
> @@ -928,9 +928,9 @@ ptransfer(int siginfo)
> if (!verbose && !siginfo)
> return;
>
> - (void)gettimeofday(&now, NULL);
> - timersub(&now, &start, &td);
> - elapsed = td.tv_sec + (td.tv_usec / 100.0);
> + clock_gettime(CLOCK_MONOTONIC, &now);
> + timespecsub(&now, &start, &td);
> + elapsed = td.tv_sec + (td.tv_nsec / 10.0);
> bs = bytes / (elapsed == 0.0 ? 1 : elapsed);
> meg = 0;
> if (bs > (1024 * 1024))
Re: refactoring of pf_find_or_create_ruleset()
Hello Hrvoje,
> Hi,
>
> with this diff i'm getting this panic:
>
> # pfctl -nvf /etc/pf.conf
> set limit states 100
> set skip on { lo em0 }
> block return all
> pass all flags S/SA
> anchor "test1" on ix1 all {
> pass all flags S/SA
> }
>
>
> # pfctl -f /etc/pf.conf
> uvm_fault(0xff07835f3100, 0x90, 0, 1) -> e
> kernel: page fault trap, code=0
> Stopped at strlcpy+0x25: movzbl 0(%rax),%edx
> ddb{3}> trace
> strlcpy(10,800023c75010,10282) at strlcpy+0x25
> pf_create_anchor(0,80cf6400) at pf_create_anchor+0xb0
> pf_find_or_create_ruleset(8145caa0) at
> pf_find_or_create_ruleset+0x1c2
> pf_anchor_setup(16,8145cdf8,8145caa0) at
> pf_anchor_setup+0x16b
> pfioctl() at pfioctl+0x31ea
thank you for for covering my back (again). I suck so much in testing, forgot
to reboot my box to see the same panic.
My earlier patch, you were testing, uncovered inconsistency between
kernel side and pfctl side of pf_main_ruleset. The inconsistency is fixed
by change as follows:
8<---8<---8<--8<
diff -r 817d4d197b87 src/sbin/pfctl/pfctl.c
--- src/sbin/pfctl/pfctl.c Thu Aug 31 21:28:31 2017 +0200
+++ src/sbin/pfctl/pfctl.c Fri Sep 01 22:47:57 2017 +0200
@@ -1572,7 +1572,6 @@ pfctl_rules(int dev, char *filename, int
RB_INIT(&pf_anchors);
memset(&pf_main_anchor, 0, sizeof(pf_main_anchor));
pf_init_ruleset(&pf_main_anchor.ruleset);
- pf_main_anchor.ruleset.anchor = &pf_main_anchor;
if (trans == NULL) {
bzero(&buf, sizeof(buf));
buf.pfrb_type = PFRB_TRANS;
diff -r 817d4d197b87 src/sys/net/pf_ruleset.c
--- src/sys/net/pf_ruleset.cThu Aug 31 21:28:31 2017 +0200
+++ src/sys/net/pf_ruleset.cFri Sep 01 22:47:57 2017 +0200
@@ -190,7 +190,7 @@ pf_create_anchor(struct pf_anchor *paren
RB_INIT(&anchor->children);
strlcpy(anchor->name, aname, sizeof(anchor->name));
- if (parent != &pf_main_anchor) {
+ if (parent != NULL) {
/*
* Make sure path for levels 2, 3, ... is terminated by '/':
* 1/2/3/...
8<---8<---8<--8<
as you can see the kernel sets ruleset.anchor to NULL (see pfattach() and then
do also a 'grep -n kludge pf_ioctl.c'), while userland links it to
pf_main_anchor.
I've remember to changing 'parent != NULL' to 'parent != &pf_main_anchor' in
pf_create_anchor() just to make regression tests passing. Fortunately you did
run my code in kernel. With change above my patch works for kernel as well as
for regression tests.
updated patch is attached.
thanks and
regards
sasha
diff -r 78ea302507cb src/sbin/pfctl/pfctl.c
--- src/sbin/pfctl/pfctl.c Thu Aug 31 21:27:47 2017 +0200
+++ src/sbin/pfctl/pfctl.c Fri Sep 01 22:52:00 2017 +0200
@@ -1572,7 +1572,6 @@ pfctl_rules(int dev, char *filename, int
RB_INIT(&pf_anchors);
memset(&pf_main_anchor, 0, sizeof(pf_main_anchor));
pf_init_ruleset(&pf_main_anchor.ruleset);
- pf_main_anchor.ruleset.anchor = &pf_main_anchor;
if (trans == NULL) {
bzero(&buf, sizeof(buf));
buf.pfrb_type = PFRB_TRANS;
diff -r 78ea302507cb src/sys/net/pf_ruleset.c
--- src/sys/net/pf_ruleset.cThu Aug 31 21:27:47 2017 +0200
+++ src/sys/net/pf_ruleset.cFri Sep 01 22:52:00 2017 +0200
@@ -133,95 +133,150 @@ pf_find_ruleset(const char *path)
}
struct pf_ruleset *
+pf_get_leaf_ruleset(char *path, char **path_remainder)
+{
+ struct pf_ruleset *ruleset;
+ char*leaf, *p;
+ int i = 0;
+
+ p = path;
+ while (*p == '/')
+ p++;
+
+ ruleset = pf_find_ruleset(p);
+ leaf = p;
+ while (ruleset == NULL) {
+ leaf = strrchr(p, '/');
+ if (leaf != NULL) {
+ *leaf = '\0';
+ i++;
+ ruleset = pf_find_ruleset(p);
+ } else {
+ leaf = path;
+ /*
+* if no path component exists, then main ruleset is
+* our parent.
+*/
+ ruleset = &pf_main_ruleset;
+ }
+ }
+
+ if (path_remainder != NULL)
+ *path_remainder = leaf;
+
+ /* restore slashes in path. */
+ while (i != 0) {
+ while (*leaf != '\0')
+ leaf++;
+ *leaf = '/';
+ i--;
+ }
+
+ return (ruleset);
+}
+
+struct pf_anchor *
+pf_create_anchor(struct pf_anchor *parent, const char *aname)
+{
+ struct pf_anchor*anchor, *dup;
+
+ if (!*aname || (strlen(aname) >= PF_ANCHOR_NAME_SIZE) ||
+ ((parent != NULL) && (strlen(parent->path) >= PF_ANCHOR_MAXPATH)))
+ return (NULL);
6.2 beta snapshot , dhclient and bridge
Unexpected behavior : GENERIC.MP#63 6.2 AMD64 (Device one) em0 <---> em5 ( Device Two <--> bridge <--> ) em0 <---> DHCP SERVER two#: dhclient em0 two#: ifconfig bridge0 create two#: ifconfig bridge0 add em5 two#: ifconfig bridge0 add em0 two#: ifconfig bridge0 up one#: dhclient em0 FAILED (paquet does not come back thourgh bridge ) two#: ifconfig em0 down two#: ifconfig vether0 create two#: dhclient vether0 FAILED two#: ifconfig em0 up two#: dhclient vether0 FAILED two#: ifconfig em0 down two#: ifconfig em0 down two#: dhclient vether0 success BUT IP on em0 and vether1 one# dhclient em0 SUCCESS -- -- - Knowing is not enough; we must apply. Willing is not enough; we must do
Re: solock() & nfs_connect
On 11/08/17(Fri) 15:32, Martin Pieuchot wrote:
> On 11/08/17(Fri) 20:31, Alexander Bluhm wrote:
> > On Fri, Aug 11, 2017 at 01:18:48PM -0400, Martin Pieuchot wrote:
> > > Diff below merge all solock()/sounlock() dances inside nfs_connect().
> >
> > Now we sleep for memory while holding the lock. Is this a good
> > idea?
Here's an alternative approach that pre-allocate mbufs. This reduce
the number of MGET() in the function an allow us to do a single
solock()/sounlock() dance.
ok?
Index: nfs//nfs_socket.c
===
RCS file: /cvs/src/sys/nfs/nfs_socket.c,v
retrieving revision 1.126
diff -u -p -r1.126 nfs_socket.c
--- nfs//nfs_socket.c 1 Sep 2017 15:05:31 - 1.126
+++ nfs//nfs_socket.c 1 Sep 2017 16:09:16 -
@@ -238,20 +238,28 @@ nfs_connect(struct nfsmount *nmp, struct
int s, error, rcvreserve, sndreserve;
struct sockaddr *saddr;
struct sockaddr_in *sin;
- struct mbuf *m;
+ struct mbuf *m = NULL, *mopt = NULL;
- if (!(nmp->nm_sotype == SOCK_DGRAM || nmp->nm_sotype == SOCK_STREAM)) {
- error = EINVAL;
- goto bad;
- }
+ if (!(nmp->nm_sotype == SOCK_DGRAM || nmp->nm_sotype == SOCK_STREAM))
+ return (EINVAL);
nmp->nm_so = NULL;
saddr = mtod(nmp->nm_nam, struct sockaddr *);
error = socreate(saddr->sa_family, &nmp->nm_so, nmp->nm_sotype,
nmp->nm_soproto);
- if (error)
- goto bad;
+ if (error) {
+ nfs_disconnect(nmp);
+ return (error);
+ }
+
+ /* Allocate mbufs possibly waiting before grabbing the socket lock. */
+ if (nmp->nm_sotype == SOCK_STREAM || saddr->sa_family == AF_INET)
+ MGET(mopt, M_WAIT, MT_SOOPTS);
+ if (saddr->sa_family == AF_INET)
+ MGET(m, M_WAIT, MT_SONAME);
+
so = nmp->nm_so;
+ s = solock(so);
nmp->nm_soflags = so->so_proto->pr_flags;
/*
@@ -260,42 +268,29 @@ nfs_connect(struct nfsmount *nmp, struct
* disclosure through UDP port capture.
*/
if (saddr->sa_family == AF_INET) {
- struct mbuf *mopt;
int *ip;
- MGET(mopt, M_WAIT, MT_SOOPTS);
mopt->m_len = sizeof(int);
ip = mtod(mopt, int *);
*ip = IP_PORTRANGE_LOW;
- s = solock(so);
error = sosetopt(so, IPPROTO_IP, IP_PORTRANGE, mopt);
- sounlock(s);
- m_freem(mopt);
if (error)
goto bad;
- MGET(m, M_WAIT, MT_SONAME);
sin = mtod(m, struct sockaddr_in *);
memset(sin, 0, sizeof(*sin));
sin->sin_len = m->m_len = sizeof(struct sockaddr_in);
sin->sin_family = AF_INET;
sin->sin_addr.s_addr = INADDR_ANY;
sin->sin_port = htons(0);
- s = solock(so);
error = sobind(so, m, &proc0);
- sounlock(s);
- m_freem(m);
if (error)
goto bad;
- MGET(mopt, M_WAIT, MT_SOOPTS);
mopt->m_len = sizeof(int);
ip = mtod(mopt, int *);
*ip = IP_PORTRANGE_DEFAULT;
- s = solock(so);
error = sosetopt(so, IPPROTO_IP, IP_PORTRANGE, mopt);
- sounlock(s);
- m_freem(mopt);
if (error)
goto bad;
}
@@ -310,12 +305,9 @@ nfs_connect(struct nfsmount *nmp, struct
goto bad;
}
} else {
- s = solock(so);
error = soconnect(so, nmp->nm_nam);
- if (error) {
- sounlock(s);
+ if (error)
goto bad;
- }
/*
* Wait for the connection to complete. Cribbed from the
@@ -328,23 +320,19 @@ nfs_connect(struct nfsmount *nmp, struct
so->so_error == 0 && rep &&
(error = nfs_sigintr(nmp, rep, rep->r_procp)) != 0){
so->so_state &= ~SS_ISCONNECTING;
- sounlock(s);
goto bad;
}
}
if (so->so_error) {
error = so->so_error;
so->so_error = 0;
- sounlock(s);
goto bad;
}
- sounlock(s);
}
/*
* Always set receive timeout to detect server crash and reconnect.
* Otherwise, we can get stuck in soreceive forever.
*/
- s = solock(so);
so->so_rcv.sb_timeo = (5 * hz);
if (nmp->nm_flag & (NFSMNT_SOFT | NFSMNT_INT))
Re: make get_last_resort_ifid() truely random
On Fri, Sep 01, 2017 at 02:20:25PM +, Florian Obser wrote:
> On Fri, Sep 01, 2017 at 03:49:47PM +0200, Stefan Sperling wrote:
> > On Fri, Sep 01, 2017 at 01:21:31PM +, Florian Obser wrote:
> > > *prod*
> >
> > As author of in6_get_rand_ifid(), I approve.
> > Your diff shall be blessed.
> >
> > Even more blessed if get_last_resort_ifid() were granted the in6_ prefix.
> > But there are more of its brothers squatting the get_ namespace, so
> > perhaps these shall all be fixed together and separately?
> >
>
> good point since I'm touching basically all the things already I added
> the in6_ prefix now. I'm also keeping the in6_get_rand_ifid() name.
>
> OK?
ok
> diff --git in6.h in6.h
> index 0caae1f586a..d80bff21370 100644
> --- in6.h
> +++ in6.h
> @@ -418,7 +418,6 @@ void in6_proto_cksum_out(struct mbuf *, struct ifnet
> *);
> int in6_localaddr(struct in6_addr *);
> int in6_addrscope(struct in6_addr *);
> struct in6_ifaddr *in6_ifawithscope(struct ifnet *, struct in6_addr *,
> u_int);
> -void in6_get_rand_ifid(struct ifnet *, struct in6_addr *);
> int in6_mask2len(struct in6_addr *, u_char *);
> int in6_nam2sin6(const struct mbuf *, struct sockaddr_in6 **);
>
> diff --git in6_ifattach.c in6_ifattach.c
> index 89acde9c6a4..6efa527128d 100644
> --- in6_ifattach.c
> +++ in6_ifattach.c
> @@ -56,10 +56,10 @@
> #include
> #endif
>
> -int get_last_resort_ifid(struct ifnet *, struct in6_addr *);
> -int get_hw_ifid(struct ifnet *, struct in6_addr *);
> -int get_ifid(struct ifnet *, struct in6_addr *);
> -int in6_ifattach_loopback(struct ifnet *);
> +void in6_get_rand_ifid(struct ifnet *, struct in6_addr *);
> +int in6_get_hw_ifid(struct ifnet *, struct in6_addr *);
> +void in6_get_ifid(struct ifnet *, struct in6_addr *);
> +int in6_ifattach_loopback(struct ifnet *);
>
> #define EUI64_GBIT 0x01
> #define EUI64_UBIT 0x02
> @@ -72,45 +72,6 @@ int in6_ifattach_loopback(struct ifnet *);
> #define IFID_LOCAL(in6) (!EUI64_LOCAL(in6))
> #define IFID_UNIVERSAL(in6) (!EUI64_UNIVERSAL(in6))
>
> -/*
> - * Generate a last-resort interface identifier, when the machine has no
> - * IEEE802/EUI64 address sources.
> - * The goal here is to get an interface identifier that is
> - * (1) random enough and (2) does not change across reboot.
> - * We currently use SHA512(hostname) for it.
> - *
> - * in6 - upper 64bits are preserved
> - */
> -int
> -get_last_resort_ifid(struct ifnet *ifp, struct in6_addr *in6)
> -{
> - SHA2_CTX ctx;
> - u_int8_t digest[SHA512_DIGEST_LENGTH];
> -
> -#if 0
> - /* we need at least several letters as seed for ifid */
> - if (hostnamelen < 3)
> - return -1;
> -#endif
> -
> - /* generate 8 bytes of pseudo-random value. */
> - SHA512Init(&ctx);
> - SHA512Update(&ctx, hostname, hostnamelen);
> - SHA512Final(digest, &ctx);
> -
> - /* assumes sizeof(digest) > sizeof(ifid) */
> - bcopy(digest, &in6->s6_addr[8], 8);
> -
> - /* make sure to set "u" bit to local, and "g" bit to individual. */
> - in6->s6_addr[8] &= ~EUI64_GBIT; /* g bit to "individual" */
> - in6->s6_addr[8] |= EUI64_UBIT; /* u bit to "local" */
> -
> - /* convert EUI64 into IPv6 interface identifier */
> - EUI64_TO_IFID(in6);
> -
> - return 0;
> -}
> -
> /*
> * Generate a random interface identifier.
> *
> @@ -135,7 +96,7 @@ in6_get_rand_ifid(struct ifnet *ifp, struct in6_addr *in6)
> * in6 - upper 64bits are preserved
> */
> int
> -get_hw_ifid(struct ifnet *ifp, struct in6_addr *in6)
> +in6_get_hw_ifid(struct ifnet *ifp, struct in6_addr *in6)
> {
> struct sockaddr_dl *sdl;
> char *addr;
> @@ -235,13 +196,13 @@ get_hw_ifid(struct ifnet *ifp, struct in6_addr *in6)
> * available on ifp0, borrow interface identifier from other information
> * sources.
> */
> -int
> -get_ifid(struct ifnet *ifp0, struct in6_addr *in6)
> +void
> +in6_get_ifid(struct ifnet *ifp0, struct in6_addr *in6)
> {
> struct ifnet *ifp;
>
> /* first, try to get it from the interface itself */
> - if (get_hw_ifid(ifp0, in6) == 0) {
> + if (in6_get_hw_ifid(ifp0, in6) == 0) {
> nd6log((LOG_DEBUG, "%s: got interface identifier from itself\n",
> ifp0->if_xname));
> goto success;
> @@ -251,7 +212,7 @@ get_ifid(struct ifnet *ifp0, struct in6_addr *in6)
> TAILQ_FOREACH(ifp, &ifnet, if_list) {
> if (ifp == ifp0)
> continue;
> - if (get_hw_ifid(ifp, in6) != 0)
> + if (in6_get_hw_ifid(ifp, in6) != 0)
> continue;
>
> /*
> @@ -267,22 +228,15 @@ get_ifid(struct ifnet *ifp0, struct in6_addr *in6)
> }
>
> /* last resort: get from random number source */
> - if (get_last_resort_ifid(ifp, in6) == 0) {
> - nd6log((LOG_DEBUG,
> - "%s: interface identifier generated by random number\n",
> - ifp0
Re: make get_last_resort_ifid() truely random
On Fri, Sep 01, 2017 at 03:49:47PM +0200, Stefan Sperling wrote:
> On Fri, Sep 01, 2017 at 01:21:31PM +, Florian Obser wrote:
> > *prod*
>
> As author of in6_get_rand_ifid(), I approve.
> Your diff shall be blessed.
>
> Even more blessed if get_last_resort_ifid() were granted the in6_ prefix.
> But there are more of its brothers squatting the get_ namespace, so
> perhaps these shall all be fixed together and separately?
>
good point since I'm touching basically all the things already I added
the in6_ prefix now. I'm also keeping the in6_get_rand_ifid() name.
OK?
diff --git in6.h in6.h
index 0caae1f586a..d80bff21370 100644
--- in6.h
+++ in6.h
@@ -418,7 +418,6 @@ voidin6_proto_cksum_out(struct mbuf *, struct ifnet
*);
intin6_localaddr(struct in6_addr *);
intin6_addrscope(struct in6_addr *);
struct in6_ifaddr *in6_ifawithscope(struct ifnet *, struct in6_addr *, u_int);
-void in6_get_rand_ifid(struct ifnet *, struct in6_addr *);
intin6_mask2len(struct in6_addr *, u_char *);
intin6_nam2sin6(const struct mbuf *, struct sockaddr_in6 **);
diff --git in6_ifattach.c in6_ifattach.c
index 89acde9c6a4..6efa527128d 100644
--- in6_ifattach.c
+++ in6_ifattach.c
@@ -56,10 +56,10 @@
#include
#endif
-int get_last_resort_ifid(struct ifnet *, struct in6_addr *);
-int get_hw_ifid(struct ifnet *, struct in6_addr *);
-int get_ifid(struct ifnet *, struct in6_addr *);
-int in6_ifattach_loopback(struct ifnet *);
+void in6_get_rand_ifid(struct ifnet *, struct in6_addr *);
+intin6_get_hw_ifid(struct ifnet *, struct in6_addr *);
+void in6_get_ifid(struct ifnet *, struct in6_addr *);
+intin6_ifattach_loopback(struct ifnet *);
#define EUI64_GBIT 0x01
#define EUI64_UBIT 0x02
@@ -72,45 +72,6 @@ int in6_ifattach_loopback(struct ifnet *);
#define IFID_LOCAL(in6)(!EUI64_LOCAL(in6))
#define IFID_UNIVERSAL(in6)(!EUI64_UNIVERSAL(in6))
-/*
- * Generate a last-resort interface identifier, when the machine has no
- * IEEE802/EUI64 address sources.
- * The goal here is to get an interface identifier that is
- * (1) random enough and (2) does not change across reboot.
- * We currently use SHA512(hostname) for it.
- *
- * in6 - upper 64bits are preserved
- */
-int
-get_last_resort_ifid(struct ifnet *ifp, struct in6_addr *in6)
-{
- SHA2_CTX ctx;
- u_int8_t digest[SHA512_DIGEST_LENGTH];
-
-#if 0
- /* we need at least several letters as seed for ifid */
- if (hostnamelen < 3)
- return -1;
-#endif
-
- /* generate 8 bytes of pseudo-random value. */
- SHA512Init(&ctx);
- SHA512Update(&ctx, hostname, hostnamelen);
- SHA512Final(digest, &ctx);
-
- /* assumes sizeof(digest) > sizeof(ifid) */
- bcopy(digest, &in6->s6_addr[8], 8);
-
- /* make sure to set "u" bit to local, and "g" bit to individual. */
- in6->s6_addr[8] &= ~EUI64_GBIT; /* g bit to "individual" */
- in6->s6_addr[8] |= EUI64_UBIT; /* u bit to "local" */
-
- /* convert EUI64 into IPv6 interface identifier */
- EUI64_TO_IFID(in6);
-
- return 0;
-}
-
/*
* Generate a random interface identifier.
*
@@ -135,7 +96,7 @@ in6_get_rand_ifid(struct ifnet *ifp, struct in6_addr *in6)
* in6 - upper 64bits are preserved
*/
int
-get_hw_ifid(struct ifnet *ifp, struct in6_addr *in6)
+in6_get_hw_ifid(struct ifnet *ifp, struct in6_addr *in6)
{
struct sockaddr_dl *sdl;
char *addr;
@@ -235,13 +196,13 @@ get_hw_ifid(struct ifnet *ifp, struct in6_addr *in6)
* available on ifp0, borrow interface identifier from other information
* sources.
*/
-int
-get_ifid(struct ifnet *ifp0, struct in6_addr *in6)
+void
+in6_get_ifid(struct ifnet *ifp0, struct in6_addr *in6)
{
struct ifnet *ifp;
/* first, try to get it from the interface itself */
- if (get_hw_ifid(ifp0, in6) == 0) {
+ if (in6_get_hw_ifid(ifp0, in6) == 0) {
nd6log((LOG_DEBUG, "%s: got interface identifier from itself\n",
ifp0->if_xname));
goto success;
@@ -251,7 +212,7 @@ get_ifid(struct ifnet *ifp0, struct in6_addr *in6)
TAILQ_FOREACH(ifp, &ifnet, if_list) {
if (ifp == ifp0)
continue;
- if (get_hw_ifid(ifp, in6) != 0)
+ if (in6_get_hw_ifid(ifp, in6) != 0)
continue;
/*
@@ -267,22 +228,15 @@ get_ifid(struct ifnet *ifp0, struct in6_addr *in6)
}
/* last resort: get from random number source */
- if (get_last_resort_ifid(ifp, in6) == 0) {
- nd6log((LOG_DEBUG,
- "%s: interface identifier generated by random number\n",
- ifp0->if_xname));
- goto success;
- }
-
- printf("%s: failed to get interface identifier\n", ifp0->if_xname);
- return -1;
-
+ in6_get_rand_ifid(ifp, in6);
+ nd6log((LOG_DEBUG,
+ "%s: interface identif
Re: isakmpd leak on error
On Fri, Sep 01, 2017 at 11:58:46AM +0200, Martin Pieuchot wrote:
> On 22/08/17(Tue) 10:50, Martin Pieuchot wrote:
> > By reviewing my last isakmpd(8) diff to fix a use-after-free, hshoexer@
> > pointed out that if exchange_establish() fails, `arg' is leaked. This is
> > not a new issue. However it generally happens under memory pressure,
> > and when you're under memory pressure leaking memory is not a clever
> > thing to do.
> >
> > In order to prevent this memory leak, we have to:
> >
> > - check for failures of exchange_establish_p{1,2}() and call the given
> >`finalize' function with the `fail' argument when this happen.
> >Because now the various finalize functions correctly free their
> >corresponding argument.
> >
> > - introduce some sanity checks in exchange_free() to be able to call
> >if even if the data structure isn't completely initialized.
> >
> > Diff below does that. It also includes a fix for a double free in one
> > of the error paths of exchange_establish().
> >
> > ok?
>
> Anyone?
The original code is clearly lacking error handling, and I cannot
spot a mistake in the error handling you've added.
ok
> >
> > Index: exchange.c
> > ===
> > RCS file: /cvs/src/sbin/isakmpd/exchange.c,v
> > retrieving revision 1.138
> > diff -u -p -r1.138 exchange.c
> > --- exchange.c 10 Mar 2016 07:32:16 - 1.138
> > +++ exchange.c 22 Aug 2017 08:45:21 -
> > @@ -529,6 +529,7 @@ exchange_enter(struct exchange *exchange
> > }
> > bucket &= bucket_mask;
> > LIST_INSERT_HEAD(&exchange_tab[bucket], exchange, link);
> > + exchange->linked = 1;
> > }
> >
> > /*
> > @@ -703,7 +704,7 @@ exchange_establish_transaction(struct ex
> > }
> >
> > /* Establish a phase 1 exchange. */
> > -void
> > +int
> > exchange_establish_p1(struct transport *t, u_int8_t type, u_int32_t doi,
> > char *name, void *args, void (*finalize)(struct exchange *, void *,
> > int),
> > void *arg, int stayalive)
> > @@ -738,7 +739,7 @@ exchange_establish_p1(struct transport *
> > else {
> > log_print("exchange_establish_p1: "
> > "DOI \"%s\" unsupported", str);
> > - return;
> > + return -1;
> > }
> >
> > /* What exchange type do we want? */
> > @@ -747,20 +748,19 @@ exchange_establish_p1(struct transport *
> > log_print("exchange_establish_p1: "
> > "no \"EXCHANGE_TYPE\" tag in [%s] section",
> > tag);
> > - return;
> > + return -1;
> > }
> > type = constant_value(isakmp_exch_cst, str);
> > if (!type) {
> > log_print("exchange_establish_p1: "
> > "unknown exchange type %s", str);
> > - return;
> > + return -1;
> > }
> > }
> > }
> > exchange = exchange_create(1, 1, doi, type);
> > if (!exchange) {
> > - /* XXX Do something here? */
> > - return;
> > + return -1;
> > }
> > if (name) {
> > exchange->name = strdup(name);
> > @@ -768,7 +768,7 @@ exchange_establish_p1(struct transport *
> > log_error("exchange_establish_p1: "
> > "strdup (\"%s\") failed", name);
> > exchange_free(exchange);
> > - return;
> > + return -1;
> > }
> > }
> > exchange->policy = name ? conf_get_str(name, "Configuration") : 0;
> > @@ -787,7 +787,7 @@ exchange_establish_p1(struct transport *
> > "calloc (1, %lu) failed",
> > (unsigned long)sizeof(*node));
> > exchange_free(exchange);
> > - return;
> > + return -1;
> > }
> > /*
> > * Insert this finalization inbetween
> > @@ -813,7 +813,7 @@ exchange_establish_p1(struct transport *
> > if (!msg) {
> > log_print("exchange_establish_p1: message_alloc () failed");
> > exchange_free(exchange);
> > - return;
> > + return 0; /* exchange_free() runs finalize */
> > }
> > msg->exchange = exchange;
> >
> > @@ -828,10 +828,9 @@ exchange_establish_p1(struct transport *
> > sa_create(exchange, 0);
> > msg->isakmp_sa = TAILQ_FIRST(&exchange->sa_list);
> > if (!msg->isakmp_sa) {
> > - /* XXX Do something more here? */
> >
Re: make get_last_resort_ifid() truely random
On Fri, Sep 01, 2017 at 01:21:31PM +, Florian Obser wrote:
> *prod*
As author of in6_get_rand_ifid(), I approve.
Your diff shall be blessed.
Even more blessed if get_last_resort_ifid() were granted the in6_ prefix.
But there are more of its brothers squatting the get_ namespace, so
perhaps these shall all be fixed together and separately?
> On Tue, Aug 15, 2017 at 09:31:26AM +, Florian Obser wrote:
> >
> > Rename in6_get_rand_ifi() to get_last_resort_ifid() and delete the old
> > get_last_resort_ifid() function because eww.
> > Also if your system is so constraint that you end up in
> > get_last_resort_ifid() you don't deserve a random ifid that stays
> > stable over reboots.
> > Simplify code a bit since get_ifid() can no longer fail.
> > It couldn't fail before either because that code path was #if 0'ed.
> >
> > OK?
> >
> > diff --git netinet6/in6.h netinet6/in6.h
> > index 0caae1f586a..d80bff21370 100644
> > --- netinet6/in6.h
> > +++ netinet6/in6.h
> > @@ -418,7 +418,6 @@ voidin6_proto_cksum_out(struct mbuf *, struct ifnet
> > *);
> > intin6_localaddr(struct in6_addr *);
> > intin6_addrscope(struct in6_addr *);
> > struct in6_ifaddr *in6_ifawithscope(struct ifnet *, struct in6_addr *,
> > u_int);
> > -void in6_get_rand_ifid(struct ifnet *, struct in6_addr *);
> > intin6_mask2len(struct in6_addr *, u_char *);
> > intin6_nam2sin6(const struct mbuf *, struct sockaddr_in6 **);
> >
> > diff --git netinet6/in6_ifattach.c netinet6/in6_ifattach.c
> > index 89acde9c6a4..a8abf5fa695 100644
> > --- netinet6/in6_ifattach.c
> > +++ netinet6/in6_ifattach.c
> > @@ -56,9 +56,9 @@
> > #include
> > #endif
> >
> > -int get_last_resort_ifid(struct ifnet *, struct in6_addr *);
> > +void get_last_resort_ifid(struct ifnet *, struct in6_addr *);
> > int get_hw_ifid(struct ifnet *, struct in6_addr *);
> > -int get_ifid(struct ifnet *, struct in6_addr *);
> > +void get_ifid(struct ifnet *, struct in6_addr *);
> > int in6_ifattach_loopback(struct ifnet *);
> >
> > #define EUI64_GBIT 0x01
> > @@ -72,52 +72,13 @@ int in6_ifattach_loopback(struct ifnet *);
> > #define IFID_LOCAL(in6)(!EUI64_LOCAL(in6))
> > #define IFID_UNIVERSAL(in6)(!EUI64_UNIVERSAL(in6))
> >
> > -/*
> > - * Generate a last-resort interface identifier, when the machine has no
> > - * IEEE802/EUI64 address sources.
> > - * The goal here is to get an interface identifier that is
> > - * (1) random enough and (2) does not change across reboot.
> > - * We currently use SHA512(hostname) for it.
> > - *
> > - * in6 - upper 64bits are preserved
> > - */
> > -int
> > -get_last_resort_ifid(struct ifnet *ifp, struct in6_addr *in6)
> > -{
> > - SHA2_CTX ctx;
> > - u_int8_t digest[SHA512_DIGEST_LENGTH];
> > -
> > -#if 0
> > - /* we need at least several letters as seed for ifid */
> > - if (hostnamelen < 3)
> > - return -1;
> > -#endif
> > -
> > - /* generate 8 bytes of pseudo-random value. */
> > - SHA512Init(&ctx);
> > - SHA512Update(&ctx, hostname, hostnamelen);
> > - SHA512Final(digest, &ctx);
> > -
> > - /* assumes sizeof(digest) > sizeof(ifid) */
> > - bcopy(digest, &in6->s6_addr[8], 8);
> > -
> > - /* make sure to set "u" bit to local, and "g" bit to individual. */
> > - in6->s6_addr[8] &= ~EUI64_GBIT; /* g bit to "individual" */
> > - in6->s6_addr[8] |= EUI64_UBIT; /* u bit to "local" */
> > -
> > - /* convert EUI64 into IPv6 interface identifier */
> > - EUI64_TO_IFID(in6);
> > -
> > - return 0;
> > -}
> > -
> > /*
> > * Generate a random interface identifier.
> > *
> > * in6 - upper 64bits are preserved
> > */
> > void
> > -in6_get_rand_ifid(struct ifnet *ifp, struct in6_addr *in6)
> > +get_last_resort_ifid(struct ifnet *ifp, struct in6_addr *in6)
> > {
> > arc4random_buf(&in6->s6_addr32[2], 8);
> >
> > @@ -235,7 +196,7 @@ get_hw_ifid(struct ifnet *ifp, struct in6_addr *in6)
> > * available on ifp0, borrow interface identifier from other information
> > * sources.
> > */
> > -int
> > +void
> > get_ifid(struct ifnet *ifp0, struct in6_addr *in6)
> > {
> > struct ifnet *ifp;
> > @@ -267,22 +228,15 @@ get_ifid(struct ifnet *ifp0, struct in6_addr *in6)
> > }
> >
> > /* last resort: get from random number source */
> > - if (get_last_resort_ifid(ifp, in6) == 0) {
> > - nd6log((LOG_DEBUG,
> > - "%s: interface identifier generated by random number\n",
> > - ifp0->if_xname));
> > - goto success;
> > - }
> > -
> > - printf("%s: failed to get interface identifier\n", ifp0->if_xname);
> > - return -1;
> > -
> > + get_last_resort_ifid(ifp, in6);
> > + nd6log((LOG_DEBUG,
> > + "%s: interface identifier generated by random number\n",
> > + ifp0->if_xname));
> > success:
> > nd6log((LOG_INFO, "%s: ifid: %02x:%02x:%02x:%02x:%02x:%02x:%02x:%02x\n",
> > ifp0->if_xname, in6->s6_addr[8], in6->s6_addr[9], in6->s6_addr[10],
Re: make get_last_resort_ifid() truely random
*prod*
On Tue, Aug 15, 2017 at 09:31:26AM +, Florian Obser wrote:
>
> Rename in6_get_rand_ifi() to get_last_resort_ifid() and delete the old
> get_last_resort_ifid() function because eww.
> Also if your system is so constraint that you end up in
> get_last_resort_ifid() you don't deserve a random ifid that stays
> stable over reboots.
> Simplify code a bit since get_ifid() can no longer fail.
> It couldn't fail before either because that code path was #if 0'ed.
>
> OK?
>
> diff --git netinet6/in6.h netinet6/in6.h
> index 0caae1f586a..d80bff21370 100644
> --- netinet6/in6.h
> +++ netinet6/in6.h
> @@ -418,7 +418,6 @@ void in6_proto_cksum_out(struct mbuf *, struct ifnet
> *);
> int in6_localaddr(struct in6_addr *);
> int in6_addrscope(struct in6_addr *);
> struct in6_ifaddr *in6_ifawithscope(struct ifnet *, struct in6_addr *,
> u_int);
> -void in6_get_rand_ifid(struct ifnet *, struct in6_addr *);
> int in6_mask2len(struct in6_addr *, u_char *);
> int in6_nam2sin6(const struct mbuf *, struct sockaddr_in6 **);
>
> diff --git netinet6/in6_ifattach.c netinet6/in6_ifattach.c
> index 89acde9c6a4..a8abf5fa695 100644
> --- netinet6/in6_ifattach.c
> +++ netinet6/in6_ifattach.c
> @@ -56,9 +56,9 @@
> #include
> #endif
>
> -int get_last_resort_ifid(struct ifnet *, struct in6_addr *);
> +void get_last_resort_ifid(struct ifnet *, struct in6_addr *);
> int get_hw_ifid(struct ifnet *, struct in6_addr *);
> -int get_ifid(struct ifnet *, struct in6_addr *);
> +void get_ifid(struct ifnet *, struct in6_addr *);
> int in6_ifattach_loopback(struct ifnet *);
>
> #define EUI64_GBIT 0x01
> @@ -72,52 +72,13 @@ int in6_ifattach_loopback(struct ifnet *);
> #define IFID_LOCAL(in6) (!EUI64_LOCAL(in6))
> #define IFID_UNIVERSAL(in6) (!EUI64_UNIVERSAL(in6))
>
> -/*
> - * Generate a last-resort interface identifier, when the machine has no
> - * IEEE802/EUI64 address sources.
> - * The goal here is to get an interface identifier that is
> - * (1) random enough and (2) does not change across reboot.
> - * We currently use SHA512(hostname) for it.
> - *
> - * in6 - upper 64bits are preserved
> - */
> -int
> -get_last_resort_ifid(struct ifnet *ifp, struct in6_addr *in6)
> -{
> - SHA2_CTX ctx;
> - u_int8_t digest[SHA512_DIGEST_LENGTH];
> -
> -#if 0
> - /* we need at least several letters as seed for ifid */
> - if (hostnamelen < 3)
> - return -1;
> -#endif
> -
> - /* generate 8 bytes of pseudo-random value. */
> - SHA512Init(&ctx);
> - SHA512Update(&ctx, hostname, hostnamelen);
> - SHA512Final(digest, &ctx);
> -
> - /* assumes sizeof(digest) > sizeof(ifid) */
> - bcopy(digest, &in6->s6_addr[8], 8);
> -
> - /* make sure to set "u" bit to local, and "g" bit to individual. */
> - in6->s6_addr[8] &= ~EUI64_GBIT; /* g bit to "individual" */
> - in6->s6_addr[8] |= EUI64_UBIT; /* u bit to "local" */
> -
> - /* convert EUI64 into IPv6 interface identifier */
> - EUI64_TO_IFID(in6);
> -
> - return 0;
> -}
> -
> /*
> * Generate a random interface identifier.
> *
> * in6 - upper 64bits are preserved
> */
> void
> -in6_get_rand_ifid(struct ifnet *ifp, struct in6_addr *in6)
> +get_last_resort_ifid(struct ifnet *ifp, struct in6_addr *in6)
> {
> arc4random_buf(&in6->s6_addr32[2], 8);
>
> @@ -235,7 +196,7 @@ get_hw_ifid(struct ifnet *ifp, struct in6_addr *in6)
> * available on ifp0, borrow interface identifier from other information
> * sources.
> */
> -int
> +void
> get_ifid(struct ifnet *ifp0, struct in6_addr *in6)
> {
> struct ifnet *ifp;
> @@ -267,22 +228,15 @@ get_ifid(struct ifnet *ifp0, struct in6_addr *in6)
> }
>
> /* last resort: get from random number source */
> - if (get_last_resort_ifid(ifp, in6) == 0) {
> - nd6log((LOG_DEBUG,
> - "%s: interface identifier generated by random number\n",
> - ifp0->if_xname));
> - goto success;
> - }
> -
> - printf("%s: failed to get interface identifier\n", ifp0->if_xname);
> - return -1;
> -
> + get_last_resort_ifid(ifp, in6);
> + nd6log((LOG_DEBUG,
> + "%s: interface identifier generated by random number\n",
> + ifp0->if_xname));
> success:
> nd6log((LOG_INFO, "%s: ifid: %02x:%02x:%02x:%02x:%02x:%02x:%02x:%02x\n",
> ifp0->if_xname, in6->s6_addr[8], in6->s6_addr[9], in6->s6_addr[10],
> in6->s6_addr[11], in6->s6_addr[12], in6->s6_addr[13],
> in6->s6_addr[14], in6->s6_addr[15]));
> - return 0;
> }
>
> /*
> @@ -318,13 +272,8 @@ in6_ifattach_linklocal(struct ifnet *ifp, struct
> in6_addr *ifid)
> ifra.ifra_addr.sin6_addr.s6_addr32[1] = 0;
> ifra.ifra_addr.sin6_addr.s6_addr[8] &= ~EUI64_GBIT;
> ifra.ifra_addr.sin6_addr.s6_addr[8] |= EUI64_UBIT;
> - } else {
> - if (get_ifid(ifp, &ifra.ifra_addr.sin6_addr) != 0) {
> -
Re: Blocking mem alloc & sosetopt()
On Fri, Sep 01, 2017 at 11:58:04AM +0200, Martin Pieuchot wrote:
> Diff below also includes another fix. Don't leave a dangling pointer
> in ip_pcbopts() by calling m_dup_pkt() as suggested by visa@. Note
> that if the allocation fails I'm returning ENOBUFS like in the IPv6
> case.
>
> ok?
OK bluhm@
>
> Index: kern/uipc_socket.c
> ===
> RCS file: /cvs/src/sys/kern/uipc_socket.c,v
> retrieving revision 1.202
> diff -u -p -r1.202 uipc_socket.c
> --- kern/uipc_socket.c22 Aug 2017 09:13:36 - 1.202
> +++ kern/uipc_socket.c22 Aug 2017 15:12:58 -
> @@ -1558,17 +1558,16 @@ 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 error = 0;
> - struct mbuf *m = m0;
>
> soassertlocked(so);
>
> if (level != SOL_SOCKET) {
> if (so->so_proto->pr_ctloutput) {
> error = (*so->so_proto->pr_ctloutput)(PRCO_SETOPT, so,
> - level, optname, m0);
> + level, optname, m);
> return (error);
> }
> error = ENOPROTOOPT;
> @@ -1576,7 +1575,7 @@ sosetopt(struct socket *so, int level, i
> switch (optname) {
> case SO_BINDANY:
> if ((error = suser(curproc, 0)) != 0) /* XXX */
> - goto bad;
> + return (error);
> break;
> }
>
> @@ -1585,10 +1584,8 @@ sosetopt(struct socket *so, int level, i
> case SO_LINGER:
> if (m == NULL || m->m_len != sizeof (struct linger) ||
> mtod(m, struct linger *)->l_linger < 0 ||
> - mtod(m, struct linger *)->l_linger > SHRT_MAX) {
> - error = EINVAL;
> - goto bad;
> - }
> + mtod(m, struct linger *)->l_linger > SHRT_MAX)
> + return (EINVAL);
> so->so_linger = mtod(m, struct linger *)->l_linger;
> /* FALLTHROUGH */
>
> @@ -1602,10 +1599,8 @@ sosetopt(struct socket *so, int level, i
> case SO_OOBINLINE:
> case SO_TIMESTAMP:
> case SO_ZEROIZE:
> - if (m == NULL || m->m_len < sizeof (int)) {
> - error = EINVAL;
> - goto bad;
> - }
> + if (m == NULL || m->m_len < sizeof (int))
> + return (EINVAL);
> if (*mtod(m, int *))
> so->so_options |= optname;
> else
> @@ -1613,10 +1608,8 @@ sosetopt(struct socket *so, int level, i
> break;
>
> case SO_DONTROUTE:
> - if (m == NULL || m->m_len < sizeof (int)) {
> - error = EINVAL;
> - goto bad;
> - }
> + if (m == NULL || m->m_len < sizeof (int))
> + return (EINVAL);
> if (*mtod(m, int *))
> error = EOPNOTSUPP;
> break;
> @@ -1628,38 +1621,28 @@ sosetopt(struct socket *so, int level, i
> {
> u_long cnt;
>
> - if (m == NULL || m->m_len < sizeof (int)) {
> - error = EINVAL;
> - goto bad;
> - }
> + if (m == NULL || m->m_len < sizeof (int))
> + return (EINVAL);
> cnt = *mtod(m, int *);
> if ((long)cnt <= 0)
> cnt = 1;
> switch (optname) {
>
> case SO_SNDBUF:
> - if (so->so_state & SS_CANTSENDMORE) {
> - error = EINVAL;
> - goto bad;
> - }
> + if (so->so_state & SS_CANTSENDMORE)
> + return (EINVAL);
> if (sbcheckreserve(cnt, so->so_snd.sb_wat) ||
> - sbreserve(so, &so->so_snd, cnt)) {
> - error = ENOBUFS;
> - goto bad;
> - }
> + sbreserve(so, &so->so_snd, cnt))
> + return (ENOBUFS);
> so->so_snd.sb_wat = cnt;
> bre
Re: isakmpd leak on error
On 22/08/17(Tue) 10:50, Martin Pieuchot wrote:
> By reviewing my last isakmpd(8) diff to fix a use-after-free, hshoexer@
> pointed out that if exchange_establish() fails, `arg' is leaked. This is
> not a new issue. However it generally happens under memory pressure,
> and when you're under memory pressure leaking memory is not a clever
> thing to do.
>
> In order to prevent this memory leak, we have to:
>
> - check for failures of exchange_establish_p{1,2}() and call the given
>`finalize' function with the `fail' argument when this happen.
>Because now the various finalize functions correctly free their
>corresponding argument.
>
> - introduce some sanity checks in exchange_free() to be able to call
>if even if the data structure isn't completely initialized.
>
> Diff below does that. It also includes a fix for a double free in one
> of the error paths of exchange_establish().
>
> ok?
Anyone?
>
> Index: exchange.c
> ===
> RCS file: /cvs/src/sbin/isakmpd/exchange.c,v
> retrieving revision 1.138
> diff -u -p -r1.138 exchange.c
> --- exchange.c10 Mar 2016 07:32:16 - 1.138
> +++ exchange.c22 Aug 2017 08:45:21 -
> @@ -529,6 +529,7 @@ exchange_enter(struct exchange *exchange
> }
> bucket &= bucket_mask;
> LIST_INSERT_HEAD(&exchange_tab[bucket], exchange, link);
> + exchange->linked = 1;
> }
>
> /*
> @@ -703,7 +704,7 @@ exchange_establish_transaction(struct ex
> }
>
> /* Establish a phase 1 exchange. */
> -void
> +int
> exchange_establish_p1(struct transport *t, u_int8_t type, u_int32_t doi,
> char *name, void *args, void (*finalize)(struct exchange *, void *, int),
> void *arg, int stayalive)
> @@ -738,7 +739,7 @@ exchange_establish_p1(struct transport *
> else {
> log_print("exchange_establish_p1: "
> "DOI \"%s\" unsupported", str);
> - return;
> + return -1;
> }
>
> /* What exchange type do we want? */
> @@ -747,20 +748,19 @@ exchange_establish_p1(struct transport *
> log_print("exchange_establish_p1: "
> "no \"EXCHANGE_TYPE\" tag in [%s] section",
> tag);
> - return;
> + return -1;
> }
> type = constant_value(isakmp_exch_cst, str);
> if (!type) {
> log_print("exchange_establish_p1: "
> "unknown exchange type %s", str);
> - return;
> + return -1;
> }
> }
> }
> exchange = exchange_create(1, 1, doi, type);
> if (!exchange) {
> - /* XXX Do something here? */
> - return;
> + return -1;
> }
> if (name) {
> exchange->name = strdup(name);
> @@ -768,7 +768,7 @@ exchange_establish_p1(struct transport *
> log_error("exchange_establish_p1: "
> "strdup (\"%s\") failed", name);
> exchange_free(exchange);
> - return;
> + return -1;
> }
> }
> exchange->policy = name ? conf_get_str(name, "Configuration") : 0;
> @@ -787,7 +787,7 @@ exchange_establish_p1(struct transport *
> "calloc (1, %lu) failed",
> (unsigned long)sizeof(*node));
> exchange_free(exchange);
> - return;
> + return -1;
> }
> /*
>* Insert this finalization inbetween
> @@ -813,7 +813,7 @@ exchange_establish_p1(struct transport *
> if (!msg) {
> log_print("exchange_establish_p1: message_alloc () failed");
> exchange_free(exchange);
> - return;
> + return 0; /* exchange_free() runs finalize */
> }
> msg->exchange = exchange;
>
> @@ -828,10 +828,9 @@ exchange_establish_p1(struct transport *
> sa_create(exchange, 0);
> msg->isakmp_sa = TAILQ_FIRST(&exchange->sa_list);
> if (!msg->isakmp_sa) {
> - /* XXX Do something more here? */
> message_free(msg);
> exchange_free(exchange);
> - return;
> + return 0; /* exchange_free() runs finalize */
> }
> sa_reference(msg->isakmp_sa);
>
> @@ -841,10 +840,11 @@ exchange_establi
Re: Blocking mem alloc & sosetopt()
On 22/08/17(Tue) 14:12, Alexander Bluhm wrote:
> On Tue, Aug 22, 2017 at 10:13:31AM +0200, Martin Pieuchot wrote:
> > @@ -1310,7 +1308,6 @@ ip_pcbopts(struct mbuf **pcbopt, struct
> > return (0);
> >
> > bad:
> > - (void)m_free(m);
> > return (EINVAL);
> > }
>
> You could replace all goto bad with return (EINVAL).
Done.
Diff below also includes another fix. Don't leave a dangling pointer
in ip_pcbopts() by calling m_dup_pkt() as suggested by visa@. Note
that if the allocation fails I'm returning ENOBUFS like in the IPv6
case.
ok?
Index: kern/uipc_socket.c
===
RCS file: /cvs/src/sys/kern/uipc_socket.c,v
retrieving revision 1.202
diff -u -p -r1.202 uipc_socket.c
--- kern/uipc_socket.c 22 Aug 2017 09:13:36 - 1.202
+++ kern/uipc_socket.c 22 Aug 2017 15:12:58 -
@@ -1558,17 +1558,16 @@ 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 error = 0;
- struct mbuf *m = m0;
soassertlocked(so);
if (level != SOL_SOCKET) {
if (so->so_proto->pr_ctloutput) {
error = (*so->so_proto->pr_ctloutput)(PRCO_SETOPT, so,
- level, optname, m0);
+ level, optname, m);
return (error);
}
error = ENOPROTOOPT;
@@ -1576,7 +1575,7 @@ sosetopt(struct socket *so, int level, i
switch (optname) {
case SO_BINDANY:
if ((error = suser(curproc, 0)) != 0) /* XXX */
- goto bad;
+ return (error);
break;
}
@@ -1585,10 +1584,8 @@ sosetopt(struct socket *so, int level, i
case SO_LINGER:
if (m == NULL || m->m_len != sizeof (struct linger) ||
mtod(m, struct linger *)->l_linger < 0 ||
- mtod(m, struct linger *)->l_linger > SHRT_MAX) {
- error = EINVAL;
- goto bad;
- }
+ mtod(m, struct linger *)->l_linger > SHRT_MAX)
+ return (EINVAL);
so->so_linger = mtod(m, struct linger *)->l_linger;
/* FALLTHROUGH */
@@ -1602,10 +1599,8 @@ sosetopt(struct socket *so, int level, i
case SO_OOBINLINE:
case SO_TIMESTAMP:
case SO_ZEROIZE:
- if (m == NULL || m->m_len < sizeof (int)) {
- error = EINVAL;
- goto bad;
- }
+ if (m == NULL || m->m_len < sizeof (int))
+ return (EINVAL);
if (*mtod(m, int *))
so->so_options |= optname;
else
@@ -1613,10 +1608,8 @@ sosetopt(struct socket *so, int level, i
break;
case SO_DONTROUTE:
- if (m == NULL || m->m_len < sizeof (int)) {
- error = EINVAL;
- goto bad;
- }
+ if (m == NULL || m->m_len < sizeof (int))
+ return (EINVAL);
if (*mtod(m, int *))
error = EOPNOTSUPP;
break;
@@ -1628,38 +1621,28 @@ sosetopt(struct socket *so, int level, i
{
u_long cnt;
- if (m == NULL || m->m_len < sizeof (int)) {
- error = EINVAL;
- goto bad;
- }
+ if (m == NULL || m->m_len < sizeof (int))
+ return (EINVAL);
cnt = *mtod(m, int *);
if ((long)cnt <= 0)
cnt = 1;
switch (optname) {
case SO_SNDBUF:
- if (so->so_state & SS_CANTSENDMORE) {
- error = EINVAL;
- goto bad;
- }
+ if (so->so_state & SS_CANTSENDMORE)
+ return (EINVAL);
if (sbcheckreserve(cnt, so->so_snd.sb_wat) ||
- sbreserve(so, &so->so_snd, cnt)) {
- error = ENOBUFS;
- goto bad;
- }
+
