Re: path mtu discovery dynamic route
On 25/06/16(Sat) 01:03, Alexander Bluhm wrote: > Hi, > > Path MTU discovery in IPv6 is slightly broken. I takes two > ICMP6_PACKET_TOO_BIG packets to create and change the dynamic route. > > When the first ICMP6 packet arrives, a dynamic route gets created > in icmp6_mtudisc_clone() and icmp6_mtudisc_update() sets its MTU > value. During notify tcp_mtudisc() removes the old route from the > inp and allocates the dynamic route. As the gateway route is not > set, the MTU is reset to 0 in _rtalloc(). > > The second packet will not allocate a dynamic route as it already > exists. Just the MTU is updated. Then tcp_mtudisc() does not > change the route at the inp as it already the correct one. So now > PMTU discovery is successful. > > This behavior was introduced in net/route.c rev 1.269 when the > gateway route allocation was moved from rt_setgateway() to _rtalloc(). > So now rtrequest() can return a route without a valid gateway route. > To fix this, I call rt_setgwroute() from _rtalloc() and rt_setgateway(). > As a result icmp6_mtudisc_clone() will create a route with a gateway > route and tcp_mtudisc() will not reset the MTU to 0 during rtalloc(). With this diff if your next hop becomes invalid after being cached you'll also need two ICMP6_PACKET_TOO_BIG to restore the MTU, is this wanted? I would prefer if we could cache the gw route in a single place. So I'm wondering if the check for reseting PMTUD makes sense (where it is). > Index: net/if_spppsubr.c > === > RCS file: /data/mirror/openbsd/cvs/src/sys/net/if_spppsubr.c,v > retrieving revision 1.154 > diff -u -p -r1.154 if_spppsubr.c > --- net/if_spppsubr.c 14 Jun 2016 20:44:43 - 1.154 > +++ net/if_spppsubr.c 24 Jun 2016 22:32:28 - > @@ -4161,7 +4161,7 @@ sppp_update_gw_walker(struct rtentry *rt > rt->rt_gateway->sa_family || > !ISSET(rt->rt_flags, RTF_GATEWAY)) > return (0); /* do not modify non-gateway routes */ > - rt_setgate(rt, rt->rt_ifa->ifa_dstaddr); > + rt_setgate(rt, rt->rt_ifa->ifa_dstaddr, ifp->if_rdomain); > } > return (0); > } > Index: net/route.c > === > RCS file: /data/mirror/openbsd/cvs/src/sys/net/route.c,v > retrieving revision 1.309 > diff -u -p -r1.309 route.c > --- net/route.c 14 Jun 2016 09:48:52 - 1.309 > +++ net/route.c 24 Jun 2016 22:37:45 - > @@ -154,6 +154,7 @@ struct pool rttimer_pool; /* pool for r > > void rt_timer_init(void); > int rt_setaddr(struct rtentry *, struct sockaddr *); > +void rt_setgwroute(struct rtentry *, u_int); > int rtflushclone1(struct rtentry *, void *, u_int); > void rtflushclone(unsigned int, struct rtentry *); > int rt_if_remove_rtdelete(struct rtentry *, void *, u_int); > @@ -369,7 +370,7 @@ rtalloc(struct sockaddr *dst, int flags, > struct rtentry * > _rtalloc(struct sockaddr *dst, uint32_t *src, int flags, unsigned int > rtableid) > { > - struct rtentry *rt, *nhrt; > + struct rtentry *rt; > > rt = rt_match(dst, src, flags, rtableid); > > @@ -381,6 +382,16 @@ _rtalloc(struct sockaddr *dst, uint32_t > if (rtisvalid(rt->rt_gwroute)) > return (rt); > > + rt_setgwroute(rt, rtableid); > + > + return (rt); > +} > + > +void > +rt_setgwroute(struct rtentry *rt, u_int rtableid) > +{ > + struct rtentry *nhrt; > + > rtfree(rt->rt_gwroute); > rt->rt_gwroute = NULL; > > @@ -392,10 +403,9 @@ _rtalloc(struct sockaddr *dst, uint32_t >* this behavior. But it is safe since rt_checkgate() wont >* allow us to us this route later on. >*/ > - nhrt = rt_match(rt->rt_gateway, NULL, flags | RT_RESOLVE, > - rtable_l2(rtableid)); > + nhrt = rt_match(rt->rt_gateway, NULL, RT_RESOLVE, rtable_l2(rtableid)); > if (nhrt == NULL) > - return (rt); > + return; > > /* >* Next hop must be reachable, this also prevents rtentry > @@ -403,13 +413,13 @@ _rtalloc(struct sockaddr *dst, uint32_t >*/ > if (ISSET(nhrt->rt_flags, RTF_CLONING|RTF_GATEWAY)) { > rtfree(nhrt); > - return (rt); > + return; > } > > /* Next hop entry must be UP and on the same interface. */ > if (!ISSET(nhrt->rt_flags, RTF_UP) || nhrt->rt_ifidx != rt->rt_ifidx) { > rtfree(nhrt); > - return (rt); > + return; > } > > /* > @@ -424,8 +434,6 @@ _rtalloc(struct sockaddr *dst, uint32_t >* do the magic for us. >*/ > rt->rt_gwroute = nhrt; > - > - return (rt); > } > > void > @@ -595,7 +603,7 @@ create: > flags |= RTF_MODIFIED; > prio = rt->rt_priority; > stat = &rtstat.rts_newgateway; > -
Re: umb(4) attachment
On 17/06/16(Fri) 22:22, Mark Kettenis wrote: > As reported earlier, umb(4) currently doesn't attach to devices that > implement both NCM 1.0 and MBIM, such as the Sierra Wireless EM7345 > that is found in some thinkpads. > > The diff below fixes this. It revamps the way we look up interface > descriptors quite a bit. I removed the unused code for matching > devices based on vendor and product ids. That code got a bit in my > way. It should be possible to bring that back if needed. > > With this fix, the EM7345 attaches as: > > umb0 at uhub0 port 4 configuration 1 interface 0 "Sierra Wireless Inc. Sierra > Wi > reless EM7345 4G LTE" rev 2.00/17.29 addr 2 > umb0: ignoring invalid segment size 1500 > umb0: vers 1.0 > umodem0 at uhub0 port 4 configuration 1 interface 2 "Sierra Wireless Inc. > Sierra Wireless EM7345 4G LTE" rev 2.00/17.29 addr 2 > umodem0: data interface 3, has no CM over data, has break > umodem0: status change notification available > ucom0 at umodem0 > > Note that it still attaches as umodem(4) as well. That is actually a > good thing since it allows me to read out the GPS though that interface. > > I believe this code should work on all devices that are properly MBIM > compliant. But of course vendors are notoriously sloppy in providing > the right usb interface descriptors for their devices. So testing is > welcome. If you run into issues, please provide lsusb -v output. Any test report? This looks good to me. > Index: if_umb.c > === > RCS file: /cvs/src/sys/dev/usb/if_umb.c,v > retrieving revision 1.1 > diff -u -p -r1.1 if_umb.c > --- if_umb.c 15 Jun 2016 19:39:34 - 1.1 > +++ if_umb.c 17 Jun 2016 20:08:05 - > @@ -204,48 +204,35 @@ const struct cfattach umb_ca = { > > int umb_delay = 4000; > > -/* > - * Normally, MBIM devices are detected by their interface class and subclass. > - * But for some models that have multiple configurations, it is better to > - * match by vendor and product id so that we can select the desired > - * configuration ourselves. > - * > - * OTOH, some devices identifiy themself als an MBIM device but fail to speak > - * the MBIM protocol. > - */ > -struct umb_products { > - struct usb_devno dev; > - int confno; > -}; > -const struct umb_products umb_devs[] = { > - /* > - * Add devices here to force them to attach as umb. > - * Format: { { VID, PID }, CONFIGNO } > - */ > -}; > - > -#define umb_lookup(vid, pid) \ > - ((const struct umb_products *)usb_lookup(umb_devs, vid, pid)) > - > int > umb_match(struct device *parent, void *match, void *aux) > { > struct usb_attach_arg *uaa = aux; > usb_interface_descriptor_t *id; > > - if (umb_lookup(uaa->vendor, uaa->product) != NULL) > - return UMATCH_VENDOR_PRODUCT; > if (!uaa->iface) > return UMATCH_NONE; > if ((id = usbd_get_interface_descriptor(uaa->iface)) == NULL) > return UMATCH_NONE; > - if (id->bInterfaceClass != UICLASS_CDC || > - id->bInterfaceSubClass != > - UISUBCLASS_MOBILE_BROADBAND_INTERFACE_MODEL || > - id->bNumEndpoints != 1) > + > + /* > + * If this function implements NCM, check if alternate setting > + * 1 implements MBIM. > + */ > + if (id->bInterfaceClass == UICLASS_CDC && > + id->bInterfaceSubClass == > + UISUBCLASS_NETWORK_CONTROL_MODEL) > + id = usbd_find_idesc(uaa->device->cdesc, uaa->ifaceno, 1); > + if (id == NULL) > return UMATCH_NONE; > > - return UMATCH_DEVCLASS_DEVSUBCLASS; > + if (id->bInterfaceClass == UICLASS_CDC && > + id->bInterfaceSubClass == > + UISUBCLASS_MOBILE_BROADBAND_INTERFACE_MODEL && > + id->bInterfaceProtocol == 0) > + return UMATCH_IFACECLASS_IFACESUBCLASS_IFACEPROTO; > + > + return UMATCH_NONE; > } > > void > @@ -257,45 +244,54 @@ umb_attach(struct device *parent, struct > struct usbd_desc_iter iter; > const usb_descriptor_t *desc; > int v; > + struct usb_cdc_union_descriptor *ud; > struct mbim_descriptor *md; > int i; > - struct usbd_interface *ctrl_iface = NULL; > int ctrl_ep; > - uint8_t data_ifaceno; > usb_interface_descriptor_t *id; > usb_config_descriptor_t *cd; > usb_endpoint_descriptor_t *ed; > + usb_interface_assoc_descriptor_t *ad; > + int current_ifaceno = -1; > + int data_ifaceno = -1; > int altnum; > int s; > struct ifnet *ifp; > int hard_mtu; > > sc->sc_udev = uaa->device; > + sc->sc_ctrl_ifaceno = uaa->ifaceno; > > - if (uaa->configno < 0) { > - /* > - * In case the device was matched by VID/PID instead of > - * InterfaceClass/InterfaceSubClass, we have to pick the > -
Re: Kernel panic pf.c during halting
On 22/06/16(Wed) 00:53, Lampshade wrote: > I don't know if this is enough, but I haven't had access to web > using other device when kernel panicked. What's the output of ifconfig and route -n show for this system? > sysctl kern.version > kern.version=OpenBSD 6.0-beta (GENERIC.MP) #2198: Sun Jun 19 11:58:45 MDT 2016 > r...@amd64.openbsd.org:/usr/src/sys/arch/amd64/compile/GENERIC.MP > > It happened after command: > sudo halt -p > > What has been written during panic > > panic:kernel diagnostic assertion "(sk->inp == NULL) || (sk->inp->inp_pf_sk > == NULL)" > failed: file "../../../net/pf.c", line 6870 > > Stopped at Debugger+0x9: leave > TID PID UID > PRFLAGS 0x100010 > PFLAGS 0x80 > CPU 0 > COMMAND pflogd > > Debugger() at Debugger+0x9 > panic() at panic+0xfe > __assert() at assert+0x25 > pf_state_key_unref() at pf_state_key_unref+0xc6 > pf_pkt_unlink_state_key() at pf_pkt_unlink_state_key+0x15 > m_free() at m_free+0xa0 > sbdrop() at sbdrop+0x80 > sbflush() at sbflush+0x1f > sbrelease() at sbrelease+0x11 > sorflush() at sorflush+0x17c > sofree() at sofree+0xbc > soclose() at soclose+0x92 > soo_close() at soo_close+0x1e (or0x1c) > fdrop() at fdrop+0x2c > end trace frame: 0x800032d01da0, count: 0 >
Re: IPV6_MINHOPCOUNT on UDP sockets
2016-06-27 20:30 GMT-03:00 Jeremie Courreges-Anglas : > Renato Westphal writes: > >> 2016-06-27 19:01 GMT-03:00 Alexander Bluhm : >>> On Mon, Jun 27, 2016 at 11:57:08PM +0200, J??r??mie Courr??ges-Anglas wrote: Alexander Bluhm writes: > The man page says IPV6_MINHOPCOUNT is only for unicast packets. > The ugly part of the code is dealing with multicast packets. IIUC Renato also needs multicast support. I thought it wouldn't be a problem to extend the IPV6_MINHOPCOUNT scope. >>> >>> If he needs it, just remove the word "unicast" from the man page. >>> Then it is OK bluhm@ > > Ack, thanks. > >> Yes, I need this to implement support for RFC 7552, which says: "(...) >> the LDP Link Hello packets MUST have their IPv6 Hop Limit set to 255, >> be checked for the same upon receipt (before any LDP-specific >> processing)". And LDP Link Hello packets are multicast UDP packets... >> >> Also, besides removing the word "unicast" from the man page, I'd go >> further and rename "datagrams" to "packets", which is a more generic >> term. > > Makes sense. Updated diff below, I'll probably commit it tomorrow > (tuesday CEST): let me know if this is enough for ldpd. Yes, that's all I need. Thank you :) -- Renato Westphal
Re: IPV6_MINHOPCOUNT on UDP sockets
Renato Westphal writes: > 2016-06-27 19:01 GMT-03:00 Alexander Bluhm : >> On Mon, Jun 27, 2016 at 11:57:08PM +0200, J??r??mie Courr??ges-Anglas wrote: >>> Alexander Bluhm writes: >>> > The man page says IPV6_MINHOPCOUNT is only for unicast packets. >>> > The ugly part of the code is dealing with multicast packets. >>> >>> IIUC Renato also needs multicast support. I thought it wouldn't be >>> a problem to extend the IPV6_MINHOPCOUNT scope. >> >> If he needs it, just remove the word "unicast" from the man page. >> Then it is OK bluhm@ Ack, thanks. > Yes, I need this to implement support for RFC 7552, which says: "(...) > the LDP Link Hello packets MUST have their IPv6 Hop Limit set to 255, > be checked for the same upon receipt (before any LDP-specific > processing)". And LDP Link Hello packets are multicast UDP packets... > > Also, besides removing the word "unicast" from the man page, I'd go > further and rename "datagrams" to "packets", which is a more generic > term. Makes sense. Updated diff below, I'll probably commit it tomorrow (tuesday CEST): let me know if this is enough for ldpd. Index: sys/netinet/udp_usrreq.c === RCS file: /cvs/src/sys/netinet/udp_usrreq.c,v retrieving revision 1.213 diff -u -p -r1.213 udp_usrreq.c --- sys/netinet/udp_usrreq.c18 Jun 2016 10:36:13 - 1.213 +++ sys/netinet/udp_usrreq.c27 Jun 2016 17:19:35 - @@ -425,15 +425,25 @@ udp_input(struct mbuf *m, ...) continue; #ifdef INET6 if (ip6) { + if (inp->inp_ip6_minhlim && + inp->inp_ip6_minhlim > ip6->ip6_hlim) + continue; if (!IN6_IS_ADDR_UNSPECIFIED(&inp->inp_laddr6)) if (!IN6_ARE_ADDR_EQUAL(&inp->inp_laddr6, &ip6->ip6_dst)) continue; } else #endif /* INET6 */ - if (inp->inp_laddr.s_addr != INADDR_ANY) { - if (inp->inp_laddr.s_addr != ip->ip_dst.s_addr) + { + if (inp->inp_ip_minttl && + inp->inp_ip_minttl > ip->ip_ttl) continue; + + if (inp->inp_laddr.s_addr != INADDR_ANY) { + if (inp->inp_laddr.s_addr != + ip->ip_dst.s_addr) + continue; + } } #ifdef INET6 if (ip6) { @@ -580,6 +590,17 @@ udp_input(struct mbuf *m, ...) } KASSERT(sotoinpcb(inp->inp_socket) == inp); +#ifdef INET6 + if (ip6 && inp->inp_ip6_minhlim && + inp->inp_ip6_minhlim > ip6->ip6_hlim) { + goto bad; + } else +#endif + if (ip && inp->inp_ip_minttl && + inp->inp_ip_minttl > ip->ip_ttl) { + goto bad; + } + #if NPF > 0 if (inp->inp_socket->so_state & SS_ISCONNECTED) pf_inp_link(m, inp); Index: share/man/man4/ip6.4 === RCS file: /cvs/src/share/man/man4/ip6.4,v retrieving revision 1.38 diff -u -p -r1.38 ip6.4 --- share/man/man4/ip6.427 Jun 2016 16:33:48 - 1.38 +++ share/man/man4/ip6.427 Jun 2016 23:24:37 - @@ -145,10 +145,8 @@ Get or set the default hop limit header datagrams sent on this socket. A value of \-1 resets to the default value. .It Dv IPV6_MINHOPCOUNT Fa "int *" -Get or set the minimum hop limit header field for incoming unicast -datagrams received on this -.Dv SOCK_STREAM -socket. +Get or set the minimum hop limit header field for incoming +packets received on this socket. This can be used to implement the .Em Generalized TTL Security Mechanism (GTSM) according to RFC 5082. -- jca | PGP: 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: IPV6_MINHOPCOUNT on UDP sockets
2016-06-27 19:01 GMT-03:00 Alexander Bluhm : > On Mon, Jun 27, 2016 at 11:57:08PM +0200, J??r??mie Courr??ges-Anglas wrote: >> Alexander Bluhm writes: >> > The man page says IPV6_MINHOPCOUNT is only for unicast packets. >> > The ugly part of the code is dealing with multicast packets. >> >> IIUC Renato also needs multicast support. I thought it wouldn't be >> a problem to extend the IPV6_MINHOPCOUNT scope. > > If he needs it, just remove the word "unicast" from the man page. > Then it is OK bluhm@ Yes, I need this to implement support for RFC 7552, which says: "(...) the LDP Link Hello packets MUST have their IPv6 Hop Limit set to 255, be checked for the same upon receipt (before any LDP-specific processing)". And LDP Link Hello packets are multicast UDP packets... Also, besides removing the word "unicast" from the man page, I'd go further and rename "datagrams" to "packets", which is a more generic term. -- Renato Westphal
Re: IPV6_MINHOPCOUNT on UDP sockets
On Mon, Jun 27, 2016 at 11:57:08PM +0200, J??r??mie Courr??ges-Anglas wrote: > Alexander Bluhm writes: > > The man page says IPV6_MINHOPCOUNT is only for unicast packets. > > The ugly part of the code is dealing with multicast packets. > > IIUC Renato also needs multicast support. I thought it wouldn't be > a problem to extend the IPV6_MINHOPCOUNT scope. If he needs it, just remove the word "unicast" from the man page. Then it is OK bluhm@
Re: IPV6_MINHOPCOUNT on UDP sockets
Alexander Bluhm writes: > On Mon, Jun 27, 2016 at 08:46:12PM +0200, Jeremie Courreges-Anglas wrote: >> >> Renato would like to implement GTSM in ldpd(8), the first step would be >> to support IPV6_MINHOPCOUNT on SOCK_DGRAM sockets. The following diff >> seems to work fine for him. >> >> I did not go down all possible *_input() methods, only regular TCP and >> UDP sockets. Is that enough to deserve the associated manpage diff? >> >> Thoughts / oks? I'll admit that the code is getting a big ugly... > > The man page says IPV6_MINHOPCOUNT is only for unicast packets. > The ugly part of the code is dealing with multicast packets. IIUC Renato also needs multicast support. I thought it wouldn't be a problem to extend the IPV6_MINHOPCOUNT scope. > I don't know which is right. Is there a specification somewhere? Nope. IPV6_MINHOPCOUNT is almost undocumented in Linux land, where it only applies to TCP sockets. -- jca | PGP: 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: Stop mesa W^X violations
Mark Kettenis writes: > As reported by several people, mesa contains code that violates W^X. > As a result glxgears aborts when using the swrast driver. The diff > below disables the offending code. The code seems to deal the absence > of W|X memory just fine. There is a fallback path that is also used > on SELinux systems. > > Note that the existing code would have worked just fine if mmap > returned MAP_FAILED for W^X violations instead of terminating the > program. Not entirely sure what the long-term plans are. [...] > Index: src/mesa/main/execmem.c > === > RCS file: /cvs/xenocara/lib/mesa/src/mesa/main/execmem.c,v > retrieving revision 1.1.1.1 > diff -u -p -r1.1.1.1 execmem.c > --- src/mesa/main/execmem.c 22 Nov 2015 02:39:37 - 1.1.1.1 > +++ src/mesa/main/execmem.c 20 Jun 2016 20:08:40 - > @@ -36,7 +36,15 @@ > > > > -#if defined(__linux__) || defined(__OpenBSD__) || defined(_NetBSD__) || > defined(__sun) || defined(__HAIKU__) > +#if defined(__OpenBSD__) > + > +static int > +init_heap(void) > +{ > + return 0; > +} The #ifdef excludes the definition of _mesa_exec_malloc and _mesa_exec_free. xlock:/usr/X11R6/lib/modules/dri/i915_dri.so: undefined symbol '_mesa_exec_malloc' xlock:/usr/X11R6/lib/modules/dri/i915_dri.so: undefined symbol '_mesa_exec_free' libGL error: unable to load driver: i915_dri.so libGL error: driver pointer missing libGL error: failed to load driver: i915 > + > +#elif defined(__linux__) || defined(_NetBSD__) || defined(__sun) || > defined(__HAIKU__) > > /* > * Allocate a large block of memory which can hold code then dole it out > -- jca | PGP: 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: rwlock for sblock
On Thu, Jun 23, 2016 at 12:44:04PM -0400, Ted Unangst wrote: > Instead of using the old flags and tsleep style lock, switch to rwlock in > sblock. That's what it's for. More legible, and as a bonus, MP safer. As mentioned by mikeb@ returning EWOULDBLOCK is important. > +void > +sbunlock(struct sockbuf *sb) > +{ > + rw_exit(&sb->sb_rwl); > +} > + > + Only one empty line please. > + struct rwlock sb_rwl; /* lock */ struct rwlock sb_lock; /* exclusive access from process */ I think calling the field lock instead of rwl makes clearer what it is. And it only controls access from process context, interrupt locking is done with splsoftnet(). A good place for a comment. enough bikeshedding, OK bluhm@
Re: rwlock for sblock
On Mon, Jun 27, 2016 at 15:32 -0400, Ted Unangst wrote: > Mike Belopuhov wrote: > > On Thu, Jun 23, 2016 at 12:44 -0400, Ted Unangst wrote: > > > Instead of using the old flags and tsleep style lock, switch to rwlock in > > > sblock. That's what it's for. More legible, and as a bonus, MP safer. > > > > > > > RW_NOSLEEP returns EBUSY if it has to wait, however > > old sblock macro would return EWOULDBLOCK (EAGAIN) > > instead. This error code is returned all the way > > to write system call so you don't want to change > > that. > > indeed. i noticed that, but wasn't sure of the implications. i checked a few > callers to see if it was directly handled, but not all the way up. > i should have mentioned it. i've added > > if (error == EBUSY) > error = EWOULDBLOCK; > Yep. LGTM.
Re: IPV6_MINHOPCOUNT on UDP sockets
On Mon, Jun 27, 2016 at 08:46:12PM +0200, Jeremie Courreges-Anglas wrote: > > Renato would like to implement GTSM in ldpd(8), the first step would be > to support IPV6_MINHOPCOUNT on SOCK_DGRAM sockets. The following diff > seems to work fine for him. > > I did not go down all possible *_input() methods, only regular TCP and > UDP sockets. Is that enough to deserve the associated manpage diff? > > Thoughts / oks? I'll admit that the code is getting a big ugly... The man page says IPV6_MINHOPCOUNT is only for unicast packets. The ugly part of the code is dealing with multicast packets. I don't know which is right. Is there a specification somewhere? bluhm > Index: sys/netinet/udp_usrreq.c > === > RCS file: /cvs/src/sys/netinet/udp_usrreq.c,v > retrieving revision 1.213 > diff -u -p -r1.213 udp_usrreq.c > --- sys/netinet/udp_usrreq.c 18 Jun 2016 10:36:13 - 1.213 > +++ sys/netinet/udp_usrreq.c 27 Jun 2016 17:19:35 - > @@ -425,15 +425,25 @@ udp_input(struct mbuf *m, ...) > continue; > #ifdef INET6 > if (ip6) { > + if (inp->inp_ip6_minhlim && > + inp->inp_ip6_minhlim > ip6->ip6_hlim) > + continue; > if (!IN6_IS_ADDR_UNSPECIFIED(&inp->inp_laddr6)) > if > (!IN6_ARE_ADDR_EQUAL(&inp->inp_laddr6, > &ip6->ip6_dst)) > continue; > } else > #endif /* INET6 */ > - if (inp->inp_laddr.s_addr != INADDR_ANY) { > - if (inp->inp_laddr.s_addr != ip->ip_dst.s_addr) > + { > + if (inp->inp_ip_minttl && > + inp->inp_ip_minttl > ip->ip_ttl) > continue; > + > + if (inp->inp_laddr.s_addr != INADDR_ANY) { > + if (inp->inp_laddr.s_addr != > + ip->ip_dst.s_addr) > + continue; > + } > } > #ifdef INET6 > if (ip6) { > @@ -580,6 +590,17 @@ udp_input(struct mbuf *m, ...) > } > KASSERT(sotoinpcb(inp->inp_socket) == inp); > > +#ifdef INET6 > + if (ip6 && inp->inp_ip6_minhlim && > + inp->inp_ip6_minhlim > ip6->ip6_hlim) { > + goto bad; > + } else > +#endif > + if (ip && inp->inp_ip_minttl && > + inp->inp_ip_minttl > ip->ip_ttl) { > + goto bad; > + } > + > #if NPF > 0 > if (inp->inp_socket->so_state & SS_ISCONNECTED) > pf_inp_link(m, inp); > Index: share/man/man4/ip6.4 > === > RCS file: /cvs/src/share/man/man4/ip6.4,v > retrieving revision 1.38 > diff -u -p -r1.38 ip6.4 > --- share/man/man4/ip6.4 27 Jun 2016 16:33:48 - 1.38 > +++ share/man/man4/ip6.4 27 Jun 2016 18:36:53 - > @@ -146,9 +146,7 @@ datagrams sent on this socket. > A value of \-1 resets to the default value. > .It Dv IPV6_MINHOPCOUNT Fa "int *" > Get or set the minimum hop limit header field for incoming unicast > -datagrams received on this > -.Dv SOCK_STREAM > -socket. > +datagrams received on this socket. > This can be used to implement the > .Em Generalized TTL Security Mechanism (GTSM) > according to RFC 5082. > > > -- > jca | PGP: 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
OpenBSD 5.9 Errata for OCSP available
This errata fixes several issues in the OCSP code that could result in the incorrect generation and parsing of OCSP requests. This remediates a lack of error checking on time parsing in these functions, and ensures that only GENERALIZEDTIME formats are accepted for OCSP, as per RFC 6960. Issues reported, and fixes provided by Kazuki Yamaguchi and Kinichiro Inoguchi Patches for OpenBSD 5.9 are available at: http://ftp.openbsd.org/pub/OpenBSD/patches/5.9/common/012_crypto.patch.sig and have been committed to -current. Portable LibreSSL releases will appear shortly.
Re: rwlock for sblock
Mike Belopuhov wrote: > On Thu, Jun 23, 2016 at 12:44 -0400, Ted Unangst wrote: > > Instead of using the old flags and tsleep style lock, switch to rwlock in > > sblock. That's what it's for. More legible, and as a bonus, MP safer. > > > > RW_NOSLEEP returns EBUSY if it has to wait, however > old sblock macro would return EWOULDBLOCK (EAGAIN) > instead. This error code is returned all the way > to write system call so you don't want to change > that. indeed. i noticed that, but wasn't sure of the implications. i checked a few callers to see if it was directly handled, but not all the way up. i should have mentioned it. i've added if (error == EBUSY) error = EWOULDBLOCK;
Re: rwlock for sblock
On Thu, Jun 23, 2016 at 12:44 -0400, Ted Unangst wrote: > Instead of using the old flags and tsleep style lock, switch to rwlock in > sblock. That's what it's for. More legible, and as a bonus, MP safer. > RW_NOSLEEP returns EBUSY if it has to wait, however old sblock macro would return EWOULDBLOCK (EAGAIN) instead. This error code is returned all the way to write system call so you don't want to change that. > > Index: kern/uipc_socket2.c > === > RCS file: /cvs/src/sys/kern/uipc_socket2.c,v > retrieving revision 1.63 > diff -u -p -r1.63 uipc_socket2.c > --- kern/uipc_socket2.c 6 Oct 2015 14:38:32 - 1.63 > +++ kern/uipc_socket2.c 23 Jun 2016 16:38:41 - > @@ -185,6 +185,9 @@ sonewconn(struct socket *head, int conns > so->so_rcv.sb_lowat = head->so_rcv.sb_lowat; > so->so_rcv.sb_timeo = head->so_rcv.sb_timeo; > > + rw_init(&so->so_rcv.sb_rwl, "sbsndl"); > + rw_init(&so->so_snd.sb_rwl, "sbrcvl"); > + > soqinsque(head, so, soqueue); > if ((*so->so_proto->pr_usrreq)(so, PRU_ATTACH, NULL, NULL, NULL, > curproc)) { > @@ -286,22 +289,24 @@ sbwait(struct sockbuf *sb) > * return any error returned from sleep (EINTR). > */ > int > -sb_lock(struct sockbuf *sb) > +sblock(struct sockbuf *sb, int wf) > { > int error; > > - while (sb->sb_flags & SB_LOCK) { > - sb->sb_flags |= SB_WANT; > - error = tsleep(&sb->sb_flags, > - (sb->sb_flags & SB_NOINTR) ? > - PSOCK : PSOCK|PCATCH, "netlck", 0); > - if (error) > - return (error); > - } > - sb->sb_flags |= SB_LOCK; > - return (0); > + error = rw_enter(&sb->sb_rwl, RW_WRITE | > + (sb->sb_flags & SB_NOINTR ? 0 : RW_INTR) | > + (wf == M_WAITOK ? 0 : RW_NOSLEEP)); > + > + return (error); > } > > +void > +sbunlock(struct sockbuf *sb) > +{ > + rw_exit(&sb->sb_rwl); > +} > + > + > /* > * Wakeup processes waiting on a socket buffer. > * Do asynchronous notification via SIGIO > @@ -827,7 +832,7 @@ void > sbflush(struct sockbuf *sb) > { > > - KASSERT((sb->sb_flags & SB_LOCK) == 0); > + rw_assert_unlocked(&sb->sb_rwl); > > while (sb->sb_mbcnt) > sbdrop(sb, (int)sb->sb_cc); > Index: sys/socketvar.h > === > RCS file: /cvs/src/sys/sys/socketvar.h,v > retrieving revision 1.60 > diff -u -p -r1.60 socketvar.h > --- sys/socketvar.h 25 Feb 2016 07:39:09 - 1.60 > +++ sys/socketvar.h 23 Jun 2016 16:40:56 - > @@ -108,13 +108,12 @@ struct socket { > struct mbuf *sb_lastrecord;/* first mbuf of last record in > socket buffer */ > struct selinfo sb_sel; /* process selecting read/write */ > + struct rwlock sb_rwl; /* lock */ > int sb_flagsintr; /* flags, changed during interrupt */ > short sb_flags; /* flags, see below */ > u_short sb_timeo; /* timeout for read/write */ > } so_rcv, so_snd; > #define SB_MAX (256*1024) /* default for max chars in > sockbuf */ > -#define SB_LOCK 0x01/* lock on data queue */ > -#define SB_WANT 0x02/* someone is waiting to lock */ > #define SB_WAIT 0x04/* someone is waiting for > data/space */ > #define SB_SEL 0x08/* someone is selecting */ > #define SB_ASYNC0x10/* ASYNC I/O, need signals */ > @@ -218,18 +217,10 @@ struct socket { > * Unless SB_NOINTR is set on sockbuf, sleep is interruptible. > * Returns error without lock if sleep is interrupted. > */ > -#define sblock(sb, wf) ((sb)->sb_flags & SB_LOCK ? \ > - (((wf) == M_WAITOK) ? sb_lock(sb) : EWOULDBLOCK) : \ > - ((sb)->sb_flags |= SB_LOCK, 0)) > +int sblock(struct sockbuf *sb, int wf); > > /* release lock on sockbuf sb */ > -#define sbunlock(sb) do { > \ > - (sb)->sb_flags &= ~SB_LOCK; \ > - if ((sb)->sb_flags & SB_WANT) { \ > - (sb)->sb_flags &= ~SB_WANT; \ > - wakeup((caddr_t)&(sb)->sb_flags); \ > - } \ > -} while (/* CONSTCOND */ 0) > +void sbunlock(struct sockbuf *sb); > > #define SB_EMPTY_FIXUP(sb) do { > \ > if ((sb)->sb_mb == NULL) { \ >
IPV6_MINHOPCOUNT on UDP sockets
Renato would like to implement GTSM in ldpd(8), the first step would be to support IPV6_MINHOPCOUNT on SOCK_DGRAM sockets. The following diff seems to work fine for him. I did not go down all possible *_input() methods, only regular TCP and UDP sockets. Is that enough to deserve the associated manpage diff? Thoughts / oks? I'll admit that the code is getting a big ugly... Index: sys/netinet/udp_usrreq.c === RCS file: /cvs/src/sys/netinet/udp_usrreq.c,v retrieving revision 1.213 diff -u -p -r1.213 udp_usrreq.c --- sys/netinet/udp_usrreq.c18 Jun 2016 10:36:13 - 1.213 +++ sys/netinet/udp_usrreq.c27 Jun 2016 17:19:35 - @@ -425,15 +425,25 @@ udp_input(struct mbuf *m, ...) continue; #ifdef INET6 if (ip6) { + if (inp->inp_ip6_minhlim && + inp->inp_ip6_minhlim > ip6->ip6_hlim) + continue; if (!IN6_IS_ADDR_UNSPECIFIED(&inp->inp_laddr6)) if (!IN6_ARE_ADDR_EQUAL(&inp->inp_laddr6, &ip6->ip6_dst)) continue; } else #endif /* INET6 */ - if (inp->inp_laddr.s_addr != INADDR_ANY) { - if (inp->inp_laddr.s_addr != ip->ip_dst.s_addr) + { + if (inp->inp_ip_minttl && + inp->inp_ip_minttl > ip->ip_ttl) continue; + + if (inp->inp_laddr.s_addr != INADDR_ANY) { + if (inp->inp_laddr.s_addr != + ip->ip_dst.s_addr) + continue; + } } #ifdef INET6 if (ip6) { @@ -580,6 +590,17 @@ udp_input(struct mbuf *m, ...) } KASSERT(sotoinpcb(inp->inp_socket) == inp); +#ifdef INET6 + if (ip6 && inp->inp_ip6_minhlim && + inp->inp_ip6_minhlim > ip6->ip6_hlim) { + goto bad; + } else +#endif + if (ip && inp->inp_ip_minttl && + inp->inp_ip_minttl > ip->ip_ttl) { + goto bad; + } + #if NPF > 0 if (inp->inp_socket->so_state & SS_ISCONNECTED) pf_inp_link(m, inp); Index: share/man/man4/ip6.4 === RCS file: /cvs/src/share/man/man4/ip6.4,v retrieving revision 1.38 diff -u -p -r1.38 ip6.4 --- share/man/man4/ip6.427 Jun 2016 16:33:48 - 1.38 +++ share/man/man4/ip6.427 Jun 2016 18:36:53 - @@ -146,9 +146,7 @@ datagrams sent on this socket. A value of \-1 resets to the default value. .It Dv IPV6_MINHOPCOUNT Fa "int *" Get or set the minimum hop limit header field for incoming unicast -datagrams received on this -.Dv SOCK_STREAM -socket. +datagrams received on this socket. This can be used to implement the .Em Generalized TTL Security Mechanism (GTSM) according to RFC 5082. -- jca | PGP: 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: rwlock for sblock
Ted Unangst wrote: > Instead of using the old flags and tsleep style lock, switch to rwlock in > sblock. That's what it's for. More legible, and as a bonus, MP safer. going once, going twice... if no oks, any objections?
Re: kill(2) and zombie processes
> From: j...@wxcvbn.org (Jeremie Courreges-Anglas) > Date: Mon, 27 Jun 2016 20:13:54 +0200 > > Some time ago someone reported a wird behavior in the shells/zsh ports. > > http://marc.info/?l=openbsd-misc&m=146101806918986&w=2 > > The latter relies on kill(zombie, 0) to succeed, instead we return -1. > This is because zombie processes aren't looked up in sys_kill(). > > A potential fix was suggested, which involved stopping using a reaper > process and let processes clean up after themselves. However such > a change doesn't look trivial, especially late in this development > cycle. > > So here's a bit of band-aid. Worth it? ok kettenis@ > Index: kern/kern_proc.c > === > RCS file: /cvs/src/sys/kern/kern_proc.c,v > retrieving revision 1.66 > diff -u -p -r1.66 kern_proc.c > --- kern/kern_proc.c 4 Mar 2016 14:09:37 - 1.66 > +++ kern/kern_proc.c 27 Jun 2016 18:02:01 - > @@ -205,6 +205,20 @@ pgfind(pid_t pgid) > } > > /* > + * Locate a zombie process > + */ > +struct process * > +zombiefind(pid_t pid) > +{ > + struct process *pr; > + > + LIST_FOREACH(pr, &zombprocess, ps_list) > + if (pr->ps_mainproc->p_pid == pid) > + return (pr); > + return (NULL); > +} > + > +/* > * Move p to a new or existing process group (and session) > * Caller provides a pre-allocated pgrp and session that should > * be freed if they are not used. > Index: kern/kern_sig.c > === > RCS file: /cvs/src/sys/kern/kern_sig.c,v > retrieving revision 1.199 > diff -u -p -r1.199 kern_sig.c > --- kern/kern_sig.c 27 Jun 2016 16:49:45 - 1.199 > +++ kern/kern_sig.c 27 Jun 2016 18:02:01 - > @@ -627,19 +627,24 @@ sys_kill(struct proc *cp, void *v, regis > int pid = SCARG(uap, pid); > int signum = SCARG(uap, signum); > int error; > + int zombie = 0; > > if ((error = pledge_kill(cp, pid)) != 0) > return (error); > if (((u_int)signum) >= NSIG) > return (EINVAL); > if (pid > 0) { > - if ((pr = prfind(pid)) == NULL) > - return (ESRCH); > + if ((pr = prfind(pid)) == NULL) { > + if ((pr = zombiefind(pid)) == NULL) > + return (ESRCH); > + else > + zombie = 1; > + } > if (!cansignal(cp, pr, signum)) > return (EPERM); > > /* kill single process */ > - if (signum) > + if (signum && !zombie) > prsignal(pr, signum); > return (0); > } > Index: sys/proc.h > === > RCS file: /cvs/src/sys/sys/proc.h,v > retrieving revision 1.223 > diff -u -p -r1.223 proc.h > --- sys/proc.h30 May 2016 21:31:27 - 1.223 > +++ sys/proc.h27 Jun 2016 18:02:01 - > @@ -482,6 +482,7 @@ pid_t allocpid(void); > void freepid(pid_t); > > struct process *prfind(pid_t); /* Find process by id. */ > +struct process *zombiefind(pid_t); /* Find zombie process by id. */ > struct proc *pfind(pid_t); /* Find thread by id. */ > struct pgrp *pgfind(pid_t); /* Find process group by id. */ > void proc_printit(struct proc *p, const char *modif, > > > -- > jca | PGP: 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE > >
kill(2) and zombie processes
Some time ago someone reported a wird behavior in the shells/zsh ports. http://marc.info/?l=openbsd-misc&m=146101806918986&w=2 The latter relies on kill(zombie, 0) to succeed, instead we return -1. This is because zombie processes aren't looked up in sys_kill(). A potential fix was suggested, which involved stopping using a reaper process and let processes clean up after themselves. However such a change doesn't look trivial, especially late in this development cycle. So here's a bit of band-aid. Worth it? Index: kern/kern_proc.c === RCS file: /cvs/src/sys/kern/kern_proc.c,v retrieving revision 1.66 diff -u -p -r1.66 kern_proc.c --- kern/kern_proc.c4 Mar 2016 14:09:37 - 1.66 +++ kern/kern_proc.c27 Jun 2016 18:02:01 - @@ -205,6 +205,20 @@ pgfind(pid_t pgid) } /* + * Locate a zombie process + */ +struct process * +zombiefind(pid_t pid) +{ + struct process *pr; + + LIST_FOREACH(pr, &zombprocess, ps_list) + if (pr->ps_mainproc->p_pid == pid) + return (pr); + return (NULL); +} + +/* * Move p to a new or existing process group (and session) * Caller provides a pre-allocated pgrp and session that should * be freed if they are not used. Index: kern/kern_sig.c === RCS file: /cvs/src/sys/kern/kern_sig.c,v retrieving revision 1.199 diff -u -p -r1.199 kern_sig.c --- kern/kern_sig.c 27 Jun 2016 16:49:45 - 1.199 +++ kern/kern_sig.c 27 Jun 2016 18:02:01 - @@ -627,19 +627,24 @@ sys_kill(struct proc *cp, void *v, regis int pid = SCARG(uap, pid); int signum = SCARG(uap, signum); int error; + int zombie = 0; if ((error = pledge_kill(cp, pid)) != 0) return (error); if (((u_int)signum) >= NSIG) return (EINVAL); if (pid > 0) { - if ((pr = prfind(pid)) == NULL) - return (ESRCH); + if ((pr = prfind(pid)) == NULL) { + if ((pr = zombiefind(pid)) == NULL) + return (ESRCH); + else + zombie = 1; + } if (!cansignal(cp, pr, signum)) return (EPERM); /* kill single process */ - if (signum) + if (signum && !zombie) prsignal(pr, signum); return (0); } Index: sys/proc.h === RCS file: /cvs/src/sys/sys/proc.h,v retrieving revision 1.223 diff -u -p -r1.223 proc.h --- sys/proc.h 30 May 2016 21:31:27 - 1.223 +++ sys/proc.h 27 Jun 2016 18:02:01 - @@ -482,6 +482,7 @@ pid_t allocpid(void); void freepid(pid_t); struct process *prfind(pid_t); /* Find process by id. */ +struct process *zombiefind(pid_t); /* Find zombie process by id. */ struct proc *pfind(pid_t); /* Find thread by id. */ struct pgrp *pgfind(pid_t);/* Find process group by id. */ void proc_printit(struct proc *p, const char *modif, -- jca | PGP: 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
fix for "ipmi0: get header fails"
ipmi(4) stopped working on my Supermicro-based machine after upgrading to 5.9. dmesg said: ipmi0 at mainbus0: version 2.0 interface KCS iobase 0xca2/2 spacing 1 ipmi0: get header fails ipmi0: no SDRs IPMI disabled The patch below fixed the problem for me. Perhaps this line was removed (in rev 1.87) by mistake. Thank you, Colin diff -u -p -u -p -r1.95 ipmi.c --- dev//ipmi.c 11 Feb 2016 04:02:22 - 1.95 +++ dev//ipmi.c 26 Jun 2016 23:24:13 - @@ -1094,6 +1094,7 @@ get_sdr_partial(struct ipmi_softc *sc, u c.c_cmd = STORAGE_GET_SDR; c.c_txlen = IPMI_SET_WDOG_MAX; c.c_rxlen = 0; + c.c_maxrxlen = 8 + length; c.c_data = cmd; ipmi_cmd(&c); len = c.c_rxlen;
Re: client certificate support in syslogd
On 27/06/16 02:02, Alexander Bluhm wrote: > On Thu, Jun 23, 2016 at 07:52:06PM +0300, Kapetanakis Giannis wrote: >> On 23/06/16 18:14, Kapetanakis Giannis wrote: >>> It adds two switches: >>> -c client_cert_file >>> -k client_key_file > > That's fine. > >>> Minor modification in CAfile setup as well to match the netcat code. > > Please do not change that now. There is a diff for libtls and > syslogd floating around that will make the code much simpler. > ... > bluhm Thanks for the comments. new version with all changes ok? Giannis Index: syslogd.8 === RCS file: /cvs/src/usr.sbin/syslogd/syslogd.8,v retrieving revision 1.40 diff -u -p -r1.40 syslogd.8 --- syslogd.8 31 Mar 2016 15:53:25 - 1.40 +++ syslogd.8 27 Jun 2016 13:53:50 - @@ -42,7 +42,9 @@ .Op Fl 46dFhnuV .Op Fl a Ar path .Op Fl C Ar CAfile +.Op Fl c Ar cert_file .Op Fl f Ar config_file +.Op Fl k Ar key_file .Op Fl m Ar mark_interval .Op Fl p Ar log_socket .Op Fl S Ar listen_address @@ -81,6 +83,9 @@ PEM encoded file containing CA certifica validation; the default is .Pa /etc/ssl/cert.pem . +.It Fl c Ar cert_file +PEM encoded file containing the client certificate for TLS connection +to a remote host. The default is not to use a certificate. .It Fl d Enable debugging to the standard output, and do not disassociate from the controlling terminal. @@ -93,6 +98,9 @@ the default is .Pa /etc/syslog.conf . .It Fl h Include the hostname when forwarding messages to a remote host. +.It Fl k Ar key_file +PEM encoded file containing the client private key for TLS connection +to a remote host. .It Fl m Ar mark_interval Select the number of minutes between .Dq mark Index: syslogd.c === RCS file: /cvs/src/usr.sbin/syslogd/syslogd.c,v retrieving revision 1.205 diff -u -p -r1.205 syslogd.c --- syslogd.c 2 Apr 2016 19:55:10 - 1.205 +++ syslogd.c 27 Jun 2016 13:53:51 - @@ -225,6 +225,8 @@ struct tls *server_ctx; struct tls_config *client_config, *server_config; const char *CAfile = "/etc/ssl/cert.pem"; /* file containing CA certificates */ intNoVerify = 0; /* do not verify TLS server x509 certificate */ +char *ClientCertfile = NULL; +char *ClientKeyfile = NULL; inttcpbuf_dropped = 0; /* count messages dropped from TCP or TLS */ #define CTL_READING_CMD1 @@ -353,7 +355,7 @@ main(int argc, char *argv[]) int ch, i; int lockpipe[2] = { -1, -1}, pair[2], nullfd, fd; - while ((ch = getopt(argc, argv, "46a:C:dFf:hm:np:S:s:T:U:uV")) != -1) + while ((ch = getopt(argc, argv, "46a:C:c:dFf:hk:m:np:S:s:T:U:uV")) != -1) switch (ch) { case '4': /* disable IPv6 */ Family = PF_INET; @@ -369,6 +371,9 @@ main(int argc, char *argv[]) case 'C': /* file containing CA certificates */ CAfile = optarg; break; + case 'c': /* file containing client certificate */ + ClientCertfile = optarg; + break; case 'd': /* debug */ Debug++; break; @@ -381,6 +386,9 @@ main(int argc, char *argv[]) case 'h': /* RFC 3164 hostnames */ IncludeHostname = 1; break; + case 'k': /* file containing client key */ + ClientKeyfile = optarg; + break; case 'm': /* mark interval */ MarkInterval = strtonum(optarg, 0, 365*24*60, &errstr); if (errstr) @@ -582,6 +590,31 @@ main(int argc, char *argv[]) free(p); close(fd); } + if (ClientCertfile && ClientKeyfile) { + uint8_t *clientcert, *clientkey; + size_t clientcertlen, clientkeylen; + + clientcert = tls_load_file(ClientCertfile, &clientcertlen, NULL); + if (clientcert == NULL) { + logerror("unable to load client TLS certificate file"); + } else if (tls_config_set_cert_mem(client_config, clientcert, + clientcertlen) == -1) { + logerror("unable to set client TLS certificate file"); + } else { + logdebug("Client cert_file %s\n", ClientCertfile); + } + clientkey = tls_load_file(ClientKeyfile, &clientkeylen, NULL); + if (clientkey == NULL) { +
Re: use inp_hops in tcp accept
Alexander Bluhm writes: > On Mon, Jun 27, 2016 at 01:16:51PM +0200, Jeremie Courreges-Anglas wrote: >> This looks much less confusing indeed. Note that this conflicts with >> your last diff to fix inp_hops. > > Here is a new version of my inp_hops diff. Note that I have moved > the assingment into the "if (inp->inp_flags & INP_IPV6)" case as > it is an IPv6 only feature. That's also how I adapted it. > ok? ok jca@ >> What I don't like is the introduction of ISSET() in this file. Wouldn't >> it be better to stay consistent? Or is ISSET() considered an >> improvement desirable enough to break consistency? > > I have commited it without ISSET() as it is more consistent when > using inp_flags. mpi@ has convinced me that ISSET() is better for > the routing code so I was in conversion mode. ack > bluhm > > Index: netinet/tcp_input.c > === > RCS file: /cvs/src/sys/netinet/tcp_input.c,v > retrieving revision 1.320 > diff -u -p -r1.320 tcp_input.c > --- netinet/tcp_input.c 27 Jun 2016 12:25:27 - 1.320 > +++ netinet/tcp_input.c 27 Jun 2016 12:42:24 - > @@ -3690,6 +3690,7 @@ syn_cache_get(struct sockaddr *src, stru > inp->inp_flags |= (oldinp->inp_flags & INP_IPV6); > if (inp->inp_flags & INP_IPV6) { > inp->inp_ipv6.ip6_hlim = oldinp->inp_ipv6.ip6_hlim; > + inp->inp_hops = oldinp->inp_hops; > } > #endif /* INET6 */ > > @@ -4346,7 +4347,7 @@ syn_cache_respond(struct syn_cache *sc, > break; > #ifdef INET6 > case AF_INET6: > - ip6->ip6_hlim = in6_selecthlim(NULL); > + ip6->ip6_hlim = in6_selecthlim(inp); > > error = ip6_output(m, NULL /*XXX*/, (struct route_in6 *)ro, 0, > NULL, NULL); > -- jca | PGP: 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: in6_selecthlim() comment
On Mon, Jun 27, 2016 at 01:08:20PM +0200, Jeremie Courreges-Anglas wrote: > > The comment is not accurate anymore. Since a few months the hop limit > that can be specified via RA is ignored. There are two ways to fix the > comment: amend it or delete it. Here, I think that the function is > trivial enough that a comment isn't needed. > > ok/nay? OK bluhm@ > > Index: in6_src.c > === > RCS file: /cvs/src/sys/netinet6/in6_src.c,v > retrieving revision 1.72 > diff -u -p -p -u -r1.72 in6_src.c > --- in6_src.c 5 Dec 2015 13:21:00 - 1.72 > +++ in6_src.c 27 Jun 2016 11:05:17 - > @@ -380,13 +380,6 @@ in6_selectif(struct sockaddr_in6 *dstsoc > return (0); > } > > -/* > - * Default hop limit selection. The precedence is as follows: > - * 1. Hoplimit value specified via ioctl. > - * 2. (If the outgoing interface is detected) the current > - * hop limit of the interface specified by router advertisement. > - * 3. The system default hoplimit. > -*/ > int > in6_selecthlim(struct inpcb *in6p) > { > > -- > jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
use inp_hops in tcp accept
On Mon, Jun 27, 2016 at 01:16:51PM +0200, Jeremie Courreges-Anglas wrote: > This looks much less confusing indeed. Note that this conflicts with > your last diff to fix inp_hops. Here is a new version of my inp_hops diff. Note that I have moved the assingment into the "if (inp->inp_flags & INP_IPV6)" case as it is an IPv6 only feature. ok? > What I don't like is the introduction of ISSET() in this file. Wouldn't > it be better to stay consistent? Or is ISSET() considered an > improvement desirable enough to break consistency? I have commited it without ISSET() as it is more consistent when using inp_flags. mpi@ has convinced me that ISSET() is better for the routing code so I was in conversion mode. bluhm Index: netinet/tcp_input.c === RCS file: /cvs/src/sys/netinet/tcp_input.c,v retrieving revision 1.320 diff -u -p -r1.320 tcp_input.c --- netinet/tcp_input.c 27 Jun 2016 12:25:27 - 1.320 +++ netinet/tcp_input.c 27 Jun 2016 12:42:24 - @@ -3690,6 +3690,7 @@ syn_cache_get(struct sockaddr *src, stru inp->inp_flags |= (oldinp->inp_flags & INP_IPV6); if (inp->inp_flags & INP_IPV6) { inp->inp_ipv6.ip6_hlim = oldinp->inp_ipv6.ip6_hlim; + inp->inp_hops = oldinp->inp_hops; } #endif /* INET6 */ @@ -4346,7 +4347,7 @@ syn_cache_respond(struct syn_cache *sc, break; #ifdef INET6 case AF_INET6: - ip6->ip6_hlim = in6_selecthlim(NULL); + ip6->ip6_hlim = in6_selecthlim(inp); error = ip6_output(m, NULL /*XXX*/, (struct route_in6 *)ro, 0, NULL, NULL);
Re: tcp syn cache new and old inp variables
Alexander Bluhm writes: > Hi, > > The variable swapping between inp, newinp and oldinpcb in syn_cache_get() > is overly complicated. Simplify the code without functional change. This looks much less confusing indeed. Note that this conflicts with your last diff to fix inp_hops. What I don't like is the introduction of ISSET() in this file. Wouldn't it be better to stay consistent? Or is ISSET() considered an improvement desirable enough to break consistency? (I personnally don't find that ISSET() helps much, but YMMV). > ok? Looks correct and works fine here, ok jca@ > bluhm > > Index: netinet/tcp_input.c > === > RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/tcp_input.c,v > retrieving revision 1.319 > diff -u -p -r1.319 tcp_input.c > --- netinet/tcp_input.c 9 Jun 2016 23:09:51 - 1.319 > +++ netinet/tcp_input.c 26 Jun 2016 23:47:27 - > @@ -3627,7 +3627,7 @@ syn_cache_get(struct sockaddr *src, stru > { > struct syn_cache *sc; > struct syn_cache_head *scp; > - struct inpcb *inp = NULL; > + struct inpcb *inp, *oldinp; > struct tcpcb *tp = NULL; > struct mbuf *am; > int s; > @@ -3670,7 +3670,8 @@ syn_cache_get(struct sockaddr *src, stru > if (so == NULL) > goto resetandabort; > > - inp = sotoinpcb(oso); > + oldinp = sotoinpcb(oso); > + inp = sotoinpcb(so); > > #ifdef IPSEC > /* > @@ -3678,30 +3679,18 @@ syn_cache_get(struct sockaddr *src, stru >* from the old pcb. Ditto for any other >* IPsec-related information. >*/ > - { > - struct inpcb *newinp = sotoinpcb(so); > - memcpy(newinp->inp_seclevel, inp->inp_seclevel, > - sizeof(inp->inp_seclevel)); > - } > + memcpy(inp->inp_seclevel, oldinp->inp_seclevel, > + sizeof(oldinp->inp_seclevel)); > #endif /* IPSEC */ > #ifdef INET6 > /* >* inp still has the OLD in_pcb stuff, set the >* v6-related flags on the new guy, too. >*/ > - { > - int flags = inp->inp_flags; > - struct inpcb *oldinpcb = inp; > - > - inp = sotoinpcb(so); > - inp->inp_flags |= (flags & INP_IPV6); > - if ((inp->inp_flags & INP_IPV6) != 0) { > - inp->inp_ipv6.ip6_hlim = > - oldinpcb->inp_ipv6.ip6_hlim; > - } > + inp->inp_flags |= (oldinp->inp_flags & INP_IPV6); > + if (ISSET(inp->inp_flags, INP_IPV6)) { > + inp->inp_ipv6.ip6_hlim = oldinp->inp_ipv6.ip6_hlim; > } > -#else /* INET6 */ > - inp = sotoinpcb(so); > #endif /* INET6 */ > > #if NPF > 0 > -- jca | PGP: 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
in6_selecthlim() comment
The comment is not accurate anymore. Since a few months the hop limit that can be specified via RA is ignored. There are two ways to fix the comment: amend it or delete it. Here, I think that the function is trivial enough that a comment isn't needed. ok/nay? Index: in6_src.c === RCS file: /cvs/src/sys/netinet6/in6_src.c,v retrieving revision 1.72 diff -u -p -p -u -r1.72 in6_src.c --- in6_src.c 5 Dec 2015 13:21:00 - 1.72 +++ in6_src.c 27 Jun 2016 11:05:17 - @@ -380,13 +380,6 @@ in6_selectif(struct sockaddr_in6 *dstsoc return (0); } -/* - * Default hop limit selection. The precedence is as follows: - * 1. Hoplimit value specified via ioctl. - * 2. (If the outgoing interface is detected) the current - * hop limit of the interface specified by router advertisement. - * 3. The system default hoplimit. -*/ int in6_selecthlim(struct inpcb *in6p) { -- jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: IP_SENDSRCADDR [2/2] : add cmsg support
Stuart Henderson writes: > On 2016/06/15 19:43, Vincent Gross wrote: >> On Mon, 13 Jun 2016 16:49:01 +0200 >> Vincent Gross wrote: >> > >> > While validating source address inside selection functions is the >> > right direction, I don't think it would be a good thing to extend >> > further in_selectsrc() prototype. However it is easy to add a check >> > while processing cmsg. >> > >> > rev2 below. Ok ? >> > >> >> rev3 below. >> >> I fixed the line length, the useless bzero(), and also the wording in >> ip.4 >> >> Ok ? > > Basically yes but one observation. I also gave my ok to vgross by IM. I know that some concerns have been exposed privately, I was not Cc'd, thus I have no idea what is the current status of that discussion. To the people concerned, please keep me / us updated about that discussion and Cc us. >> Index: sys/netinet/in.h >> === >> RCS file: /cvs/src/sys/netinet/in.h,v >> retrieving revision 1.115 >> diff -u -p -r1.115 in.h >> --- sys/netinet/in.h 20 Oct 2015 20:22:42 - 1.115 >> +++ sys/netinet/in.h 15 Jun 2016 17:37:11 - >> @@ -307,6 +307,7 @@ struct ip_opts { >> #define IP_RECVRTABLE 35 /* bool; receive rdomain w/dgram */ >> #define IP_IPSECFLOWINFO36 /* bool; IPsec flow info for dgram */ >> #define IP_IPDEFTTL 37 /* int; IP TTL system default */ >> +#define IP_SENDSRCADDR 38 /* struct in_addr; source address >> to use */ > > Other OS with this have it at the same value as IP_RECVDSTADDR. > Not doing that currently breaks net/gdnsd - I can take care of that > but I just wanted to flag it up as a difference to other implementations. > > So as long as that doesn't cause any concern, OK sthen@, and I will take > care of bumps etc as necessary in ports. I think it would be better to match what other implementations (FreeBSD) do. I can't think of a negative impact. -- jca | PGP: 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE