On Wed, Jun 27 2018, Vincent Gross <vgr...@openbsd.org> wrote:
> So a while back Alexander Markert sent a bug report regarding sendmsg()
> behaviour with IP_SENDSRCADDR :
>
> https://marc.info/?l=openbsd-tech&m=149276833923905&w=2
>
> This impacts our dnsmasq port :
>
> https://marc.info/?l=openbsd-tech&m=149234052220818&w=2
>
> Alexander Markert shows in the first thread the problematic code and
> conditions.
>
> To save you the trip back in time : sendfrom() returns EWOULDBLOCK (or
> blocks if using blocking IO) when len(cmsg) + len(data) >
> len(socket.buffer). The better behaviour would be to never block and
> return EMSGSIZE.
>
> The first diff fixes the kernel code ; The second diff reverts
> https://marc.info/?l=openbsd-ports-cvs&m=149233921320572&w=2 and fixes
> a bad cmsg setup.
>
> 1) Can you confirm this fixes dnsmasq ? or whatever you used to trigger
>   the bug ?

The dnsmasq problem is unrelated to your uipc_socket.c diff.  Your
dnsmasq diff is almost enough to fix dnsmasq operations in all cases,
and looks a lot like the diff I sent last year to Brad and Stuart.
DNS requests sent to a IPv6 link-local addresses are broken (after
applying the diff), so another tweak is probably needed.

One big problem to debug this specific issue is that EINVAL is ignored
because of an error that supposedly happens on Linux only. Please find
an updated diff at the end of this email.

> 2) Ok ?

I don't know whether this is the proper approach, but your diff goes
in the opposite direction of the changes done in revision 1.145.

> (apologies for the delay by the way :S )

I'm way more guilty than you are here, since I've postponed this problem
while pretending several times that I would publish a diff soon(tm).  So
first, thank you for trying to unblock this, and apologies for my
unprofessional behavior.

Now, about the issue at hand:

--8<--
                space = sbspace(so, &so->so_snd);
                if (flags & MSG_OOB)
                        space += 1024;
                if ((atomic && resid > so->so_snd.sb_hiwat) ||
                    (so->so_proto->pr_domain->dom_family != AF_UNIX &&
                    clen > so->so_snd.sb_hiwat))
                        snderr(EMSGSIZE);
                if (space < clen ||
                    (space - clen < resid &&
                    (atomic || space < so->so_snd.sb_lowat))) {
                        if ((so->so_state & SS_NBIO) || (flags & MSG_DONTWAIT))
                                snderr(EWOULDBLOCK);
                        sbunlock(so, &so->so_snd);
                        error = sbwait(so, &so->so_snd);
                        so->so_state &= ~SS_ISSENDING;
                        if (error)
                                goto out;
                        goto restart;
                }
                space -= clen;
-->8--

I hate those conditions.  I do not fully understand their true intent
and I feel they are already brittle; I'm afraid that adding more
conditionals will worsen clarity.  How are those three (actually way
more than three) conditions actually supposed to interact?

If people have a clearer idea than I do about what exactly should happen
here, please share.


Updated dnsmasq diff (should probably be discussed on ports@)

Index: patches/patch-src_dnsmasq_c
===================================================================
RCS file: patches/patch-src_dnsmasq_c
diff -N patches/patch-src_dnsmasq_c
--- patches/patch-src_dnsmasq_c 29 Mar 2018 19:42:51 -0000      1.5
+++ /dev/null   1 Jan 1970 00:00:00 -0000
@@ -1,16 +0,0 @@
-$OpenBSD: patch-src_dnsmasq_c,v 1.5 2018/03/29 19:42:51 ajacoutot Exp $
-
-Fails. Currently disabled pending investigation.
-
-Index: src/dnsmasq.c
---- src/dnsmasq.c.orig
-+++ src/dnsmasq.c
-@@ -149,7 +149,7 @@ int main (int argc, char **argv)
-       open("/dev/null", O_RDWR); 
- 
- #ifndef HAVE_LINUX_NETWORK
--#  if !(defined(IP_RECVDSTADDR) && defined(IP_RECVIF) && 
defined(IP_SENDSRCADDR))
-+#  if defined(__OpenBSD__) || !(defined(IP_RECVDSTADDR) && defined(IP_RECVIF) 
&& defined(IP_SENDSRCADDR))
-   if (!option_bool(OPT_NOWILD))
-     {
-       bind_fallback = 1;
Index: patches/patch-src_forward_c
===================================================================
RCS file: /d/cvs/ports/net/dnsmasq/patches/patch-src_forward_c,v
retrieving revision 1.1
diff -u -p -r1.1 patch-src_forward_c
--- patches/patch-src_forward_c 16 Apr 2017 10:40:07 -0000      1.1
+++ patches/patch-src_forward_c 12 Jul 2018 15:22:41 -0000
@@ -1,24 +1,35 @@
 $OpenBSD: patch-src_forward_c,v 1.1 2017/04/16 10:40:07 sthen Exp $
 
-Fails. Currently disabled pending investigation.
+Fix invalid cmsg lengths.
 
---- src/forward.c.orig Sat Apr 15 22:36:04 2017
-+++ src/forward.c      Sat Apr 15 22:46:09 2017
-@@ -35,7 +35,7 @@ int send_from(int fd, int nowild, char *packet, size_t
-     struct cmsghdr align; /* this ensures alignment */
- #if defined(HAVE_LINUX_NETWORK)
-     char control[CMSG_SPACE(sizeof(struct in_pktinfo))];
--#elif defined(IP_SENDSRCADDR)
-+#elif !defined(__OpenBSD__) && defined(IP_SENDSRCADDR)
-     char control[CMSG_SPACE(sizeof(struct in_addr))];
- #endif
- #ifdef HAVE_IPV6
-@@ -71,7 +71,7 @@ int send_from(int fd, int nowild, char *packet, size_t
-         msg.msg_controllen = cmptr->cmsg_len = CMSG_LEN(sizeof(struct 
in_pktinfo));
-         cmptr->cmsg_level = IPPROTO_IP;
+Index: src/forward.c
+--- src/forward.c.orig
++++ src/forward.c
+@@ -73,7 +73,8 @@ int send_from(int fd, int nowild, char *packet, size_t
          cmptr->cmsg_type = IP_PKTINFO;
--#elif defined(IP_SENDSRCADDR)
-+#elif !defined(__OpenBSD__) && defined(IP_SENDSRCADDR)
+ #elif defined(IP_SENDSRCADDR)
          memcpy(CMSG_DATA(cmptr), &(source->addr.addr4), 
sizeof(source->addr.addr4));
-         msg.msg_controllen = cmptr->cmsg_len = CMSG_LEN(sizeof(struct 
in_addr));
+-        msg.msg_controllen = cmptr->cmsg_len = CMSG_LEN(sizeof(struct 
in_addr));
++        msg.msg_controllen = sizeof(control_u.control);
++        cmptr->cmsg_len = CMSG_LEN(sizeof(struct in_addr));
          cmptr->cmsg_level = IPPROTO_IP;
+         cmptr->cmsg_type = IP_SENDSRCADDR;
+ #endif
+@@ -83,6 +84,7 @@ int send_from(int fd, int nowild, char *packet, size_t
+       {
+         struct in6_pktinfo p;
+         p.ipi6_ifindex = iface; /* Need iface for IPv6 to handle link-local 
addrs */
++        my_syslog(LOG_ERR, _("%s: p.ipi6_ifindex == %u"), __func__, 
p.ipi6_ifindex);
+         p.ipi6_addr = source->addr.addr6;
+         memcpy(CMSG_DATA(cmptr), &p, sizeof(p));
+         msg.msg_controllen = cmptr->cmsg_len = CMSG_LEN(sizeof(struct 
in6_pktinfo));
+@@ -96,8 +98,7 @@ int send_from(int fd, int nowild, char *packet, size_t
+   
+   while (retry_send(sendmsg(fd, &msg, 0)));
+ 
+-  /* If interface is still in DAD, EINVAL results - ignore that. */
+-  if (errno != 0 && errno != EINVAL)
++  if (errno != 0)
+     {
+       my_syslog(LOG_ERR, _("failed to send packet: %s"), strerror(errno));
+       return 0;


-- 
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE

Reply via email to