Re: umb(4) attachment

[Martin Pieuchot]

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

[Martin Pieuchot]

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

[Renato Westphal]

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

[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.

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_laddr6))
if 
(!IN6_ARE_ADDR_EQUAL(>inp_laddr6,
>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

[Renato Westphal]

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

[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@

Re: IPV6_MINHOPCOUNT on UDP sockets

[Jérémie Courrèges-Anglas]

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

[Jeremie Courreges-Anglas]

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

[Alexander Bluhm]

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_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

[Mike Belopuhov]

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

[Alexander Bluhm]

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_laddr6))
>   if 
> (!IN6_ARE_ADDR_EQUAL(>inp_laddr6,
>   >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

[Bob Beck]

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

[Ted Unangst]

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

[Mike Belopuhov]

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_rcv.sb_rwl, "sbsndl");
> + rw_init(>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_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_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_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_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

[Jeremie Courreges-Anglas]

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_laddr6))
if 
(!IN6_ARE_ADDR_EQUAL(>inp_laddr6,
>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]

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

[Mark Kettenis]

> 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=146101806918986=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, , 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

[Jeremie Courreges-Anglas]

Some time ago someone reported a wird behavior in the shells/zsh ports.

  http://marc.info/?l=openbsd-misc=146101806918986=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, , 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"

[Colin Stolley]

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();
len = c.c_rxlen;

Re: client certificate support in syslogd

[Kapetanakis Giannis]

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, );
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, 
, 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, , 
NULL);
+   if (clientkey == NULL) {
+   logerror("unable to load 

Re: use inp_hops in tcp accept

[Jeremie Courreges-Anglas]

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

[Alexander Bluhm]

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

[Alexander Bluhm]

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

[Jeremie Courreges-Anglas]

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

[Jeremie Courreges-Anglas]

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

[Jérémie Courrèges-Anglas]

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

Re: cat.1 forward refs

[Jason McIntyre]

On Sun, Jun 26, 2016 at 09:30:15PM -0400, Ted Unangst wrote:
> I find it confusing to read a man page that constantly tells me that this
> option is like some other option. I have to go read the other option, then
> return here and reparse what's different. In my unimpeachable opinion, the
> text for each option should be self contained, with references to other
> options only if the redudant text would be too verbose.
> 
> cat.1 has several examples of this. Saying "Implies the -n option" is longer
> than simply telling me "Number the lines" and conveys much less information.
> 
> This reworks the page to briefly explain each option.
> 

morning.

i think you're right that "implies the -n option" is limited in the
information it conveys. but... not mentioning that an option effectively
sets another option opens up a big ambiguity for the reader. -b numbers
the line, so do i not need to set -n? is it exactly the same?

therefore the pattern is currently to say it explicitly. we could
do both, but that adds verbosity. it's a trade off, no? i wouldn;t
be against documenting both bits of information, but i'm not sure
it's really worth it.

i think it'd be wrong to lose the info about options implying other
options though. and i note where you now effectively do both (-e and -t)
you are at least as verbose as before, if not more. and "as according
to" is much less clear than "implies".

jmc

> Index: cat.1
> ===
> RCS file: /cvs/src/bin/cat/cat.1,v
> retrieving revision 1.35
> diff -u -p -r1.35 cat.1
> --- cat.1 4 Nov 2015 21:28:01 -   1.35
> +++ cat.1 27 Jun 2016 01:25:12 -
> @@ -61,25 +61,23 @@ reads from the standard input.
>  The options are as follows:
>  .Bl -tag -width Ds
>  .It Fl b
> -Implies the
> -.Fl n
> -option but doesn't count blank lines.
> +Number the lines, but don't count blank lines.
>  .It Fl e
> -Implies the
> -.Fl v
> -option and also prints a dollar sign
> +Print a dollar sign
>  .Pq Ql \&$
>  at the end of each line.
> +Also displays non-printing characters as according to
> +.Fl v .
>  .It Fl n
>  Number the output lines, starting at 1.
>  .It Fl s
>  Squeeze multiple adjacent empty lines, causing the output to be
>  single spaced.
>  .It Fl t
> -Implies the
> -.Fl v
> -option and also prints tab characters as
> +Print tab characters as
>  .Ql ^I .
> +Also displays other non-printing characters as according to
> +.Fl v .
>  .It Fl u
>  The output is guaranteed to be unbuffered (see
>  .Xr setvbuf 3 ) .
> @@ -89,9 +87,6 @@ Control characters print as
>  .Ql ^X
>  for control-X, with the exception of the tab and EOL characters,
>  which are displayed normally.
> -The tab character, control-I, can be made visible via the
> -.Fl t
> -option.
>  The DEL character (octal 0177) prints as
>  .Ql ^? .
>  Non-ASCII characters (with the high bit set) are printed as
>