Re: freetype bumps [Re: CVS: cvs.openbsd.org: xenocara]

[Stuart Henderson]

On 2018/05/25 22:55, Matthieu Herrb wrote:
> On Fri, May 25, 2018 at 12:11:27PM +0100, Stuart Henderson wrote:
> > 
> > Some packages depend on freetype AND another X lib that depends on freetype,
> > Until new packages are available and the user has run pkg_add -u, this 
> > results
> > in two conflicting versions of freetype being pulled in.
> > 
> > WANTLIB is not the problem. WANTLIBs are correct. The packages do get
> > updated once new packages are available, the problem is in the interim
> > between bumping libs and new packages becoming available.
> > 
> > Build times:
> > 
> > aarch64 ~4 days
> > amd64   1 day
> > arm ~10 days
> > hppa~4 days
> > i38628-50 hours
> > mips64  4 days
> > mips64el6 weeks
> > powerpc ~2..3 weeks
> > sparc64 ~2..3 weeks
> > 
> > But unless bulk builders know that they need to restart builds, the previous
> > build needs to finish before the new one can start, so it can be double 
> > those
> > times before packages built against new libs are shipped.
> > 
> > I don't know what the case is with libc/libm. We *do* do this type of
> > bump in libssl/libcrypto/libtls because we know things break otherwise.
> > And there is a clear problem with the freetype chain because it keeps
> > happening again and again.
> > 
> > By bumping the other libs e.g. fontconfig, already-installed software
> > will continue to use the version of the other libs already on their
> > disk, which uses the old freetype, avoiding the conflict.
> >
> 
> Ok, thanks. I keep forgetting about this delay in builing packages.
> In that case I understand that it will help.
> 
> So ok for the following diff and a (late) bump of the 3 mentionned
> libs ?
> 
> Index: shlib_version
> ===
> RCS file: /cvs/OpenBSD/xenocara/lib/freetype/shlib_version,v
> retrieving revision 1.28
> diff -u -r1.28 shlib_version
> --- shlib_version 21 May 2018 11:52:24 -  1.28
> +++ shlib_version 25 May 2018 20:54:40 -
> @@ -1,2 +1,4 @@
>  major=29
>  minor=0
> +# note: If bumping the major, also bump major for libXft, libXfont2
> +#   and libfontconfig 
> 
> -- 
> Matthieu Herrb

Bumping would certainly help. Arches currently building packages look like this:

$ grep libfreetype.so. */*sig.log
aarch64/arm64-1.sig.log:/usr/X11R6/lib/libfreetype.so.29.0
aarch64/localhost.sig.log:  /usr/X11R6/lib/libfreetype.so.29.0
arm/armv7-1.sig.log:/usr/X11R6/lib/libfreetype.so.28.2
arm/armv7-2.sig.log:/usr/X11R6/lib/libfreetype.so.28.2
arm/localhost.sig.log:  /usr/X11R6/lib/libfreetype.so.28.2
hppa/hppa-1.sig.log:/usr/X11R6/lib/libfreetype.so.28.2
hppa/localhost.sig.log: /usr/X11R6/lib/libfreetype.so.28.2
i386/i386-2.sig.log:/usr/X11R6/lib/libfreetype.so.29.0
i386/i386.sig.log:  /usr/X11R6/lib/libfreetype.so.29.0
i386/localhost.sig.log: /usr/X11R6/lib/libfreetype.so.29.0
powerpc/localhost.sig.log:  /usr/X11R6/lib/libfreetype.so.28.2
powerpc/macppc-0.sig.log:   /usr/X11R6/lib/libfreetype.so.28.2
sparc64/localhost.sig.log:  /usr/X11R6/lib/libfreetype.so.28.2
sparc64/sparc64-0.sig.log:  /usr/X11R6/lib/libfreetype.so.28.2
sparc64/sparc64-2.sig.log:  /usr/X11R6/lib/libfreetype.so.28.2

only fast arches already have 29.0 so won't be too badly hurt by
bumping the other libraries now, and will help the slower-building
arches. So OK with bumps.

For the comment, I'm unsure what is needed when the minor is bumped,
it depends whether ld.so picks up an exact match if available, or
if it always looks for the highest minor version. The text we have
in libcrypto's shlib_version is "Don't forget to give libssl and
libtls the same type of bump!" so at least at some time somebody thought
it was needed for minor bumps too. Someone else reading can probably
give better advice on this.

Re: freetype bumps [Re: CVS: cvs.openbsd.org: xenocara]

[Matthieu Herrb]

On Fri, May 25, 2018 at 12:11:27PM +0100, Stuart Henderson wrote:
> 
> Some packages depend on freetype AND another X lib that depends on freetype,
> Until new packages are available and the user has run pkg_add -u, this results
> in two conflicting versions of freetype being pulled in.
> 
> WANTLIB is not the problem. WANTLIBs are correct. The packages do get
> updated once new packages are available, the problem is in the interim
> between bumping libs and new packages becoming available.
> 
> Build times:
> 
> aarch64 ~4 days
> amd64   1 day
> arm ~10 days
> hppa~4 days
> i38628-50 hours
> mips64  4 days
> mips64el6 weeks
> powerpc ~2..3 weeks
> sparc64 ~2..3 weeks
> 
> But unless bulk builders know that they need to restart builds, the previous
> build needs to finish before the new one can start, so it can be double those
> times before packages built against new libs are shipped.
> 
> I don't know what the case is with libc/libm. We *do* do this type of
> bump in libssl/libcrypto/libtls because we know things break otherwise.
> And there is a clear problem with the freetype chain because it keeps
> happening again and again.
> 
> By bumping the other libs e.g. fontconfig, already-installed software
> will continue to use the version of the other libs already on their
> disk, which uses the old freetype, avoiding the conflict.
>

Ok, thanks. I keep forgetting about this delay in builing packages.
In that case I understand that it will help.

So ok for the following diff and a (late) bump of the 3 mentionned
libs ?

Index: shlib_version
===
RCS file: /cvs/OpenBSD/xenocara/lib/freetype/shlib_version,v
retrieving revision 1.28
diff -u -r1.28 shlib_version
--- shlib_version   21 May 2018 11:52:24 -  1.28
+++ shlib_version   25 May 2018 20:54:40 -
@@ -1,2 +1,4 @@
 major=29
 minor=0
+# note: If bumping the major, also bump major for libXft, libXfont2
+#   and libfontconfig 

-- 
Matthieu Herrb

fix libepoxy build on mips64el

[Matthieu Herrb]

Hi,

after the update to 1.5.2 libepoxy fails to link on loongson with:
gl_generated_dispatch.c:(.text+0x88): relocation truncated to fit: 
R_MIPS_CALL16 against `epoxy_is_desktop_gl'
and other similar errors.

This fixes it. ok ?

Index: Makefile
===
RCS file: /cvs/OpenBSD/xenocara/lib/libepoxy/Makefile,v
retrieving revision 1.7
diff -u -p -u -r1.7 Makefile
--- Makefile25 May 2018 06:24:59 -  1.7
+++ Makefile25 May 2018 20:00:34 -
@@ -13,6 +13,10 @@ INCSDIR= ${X11BASE}/include/epoxy
 
 CFLAGS+= -std=gnu99
 
+.if ${MACHINE_ARCH} == "mips64el"
+PICFLAG+= -mxgot
+.endif
+
 CPPFLAGS+= \
-I${.CURDIR} \
-I${.CURDIR}/generated/include \

-- 
Matthieu Herrb

Re: [Patch] mg(1) tutorial: no columns :(

[Leonid Bobrov]

> Then why not write that diff?

Because I see setting column-number-mode on by default as best solution,
but you have/had nice reason to set it to off by default. Ok, here's
diff:

Index: display.c
===
RCS file: /cvs/src/usr.bin/mg/display.c,v
retrieving revision 1.48
diff -u -p -u -p -r1.48 display.c
--- display.c   6 Jul 2017 19:27:37 -   1.48
+++ display.c   25 May 2018 19:31:33 -
@@ -91,7 +91,7 @@ struct videoblanks;   /* Blank line im
 struct score *score;   /* [NROW * NROW] */
 
 static int  linenos = TRUE;
-static int  colnos = FALSE;
+static int  colnos = TRUE;
 
 /* Is macro recording enabled? */
 extern int macrodef;

Re: [patch] Skip background scan if bssid is set

[Jesper Wallin]

Hi Stefan,

Unfortunately, it makes no difference. :-/

With bssid specified:
  25 packets transmitted, 25 packets received, 0.0% packet loss
  round-trip min/avg/max/std-dev = 7.357/8.340/25.425/3.489 ms

Without bssid specified:
  25 packets transmitted, 20 packets received, 20.0% packet loss
  round-trip min/avg/max/std-dev = 7.344/16.816/118.988/25.672 ms

When testing for longer periods, I still see some packet loss even with
the bssid specified, but it's a lot worse without it specified and is
timed with when the background scan starts.

On Fri, May 25, 2018 at 07:47:26PM +0200, Stefan Sperling wrote:
> On Sun, Apr 29, 2018 at 03:39:07AM +0200, Jesper Wallin wrote:
> > Hi all,
> > 
> > I recently learned that my AP behaves badly and I have packet loss when
> > the background scan is running.
> 
> Hi Jesper,
> 
> Can you please check if this diff makes your AP work without
> hardcoding its bssid?
> 
> Index: if_iwm.c
> ===
> RCS file: /cvs/src/sys/dev/pci/if_iwm.c,v
> retrieving revision 1.230
> diff -u -p -r1.230 if_iwm.c
> --- if_iwm.c  23 May 2018 17:49:20 -  1.230
> +++ if_iwm.c  25 May 2018 17:32:41 -
> @@ -4719,7 +4719,7 @@ iwm_lmac_scan_fill_channels(struct iwm_s
>   chan->iter_count = htole16(1);
>   chan->iter_interval = 0;
>   chan->flags = htole32(IWM_UNIFIED_SCAN_CHANNEL_PARTIAL);
> - if (n_ssids != 0 && !bgscan)
> + if (n_ssids != 0)
>   chan->flags |= htole32(1 << 1); /* select SSID 0 */
>   chan++;
>   nchan++;
> @@ -4746,7 +4746,7 @@ iwm_umac_scan_fill_channels(struct iwm_s
>   chan->channel_num = ieee80211_mhz2ieee(c->ic_freq, 0);
>   chan->iter_count = 1;
>   chan->iter_interval = htole16(0);
> - if (n_ssids != 0 && !bgscan)
> + if (n_ssids != 0)
>   chan->flags = htole32(1 << 0); /* select SSID 0 */
>   chan++;
>   nchan++;
> @@ -4898,9 +4898,6 @@ iwm_lmac_scan(struct iwm_softc *sc, int 
>   IWM_LMAC_SCAN_FLAG_EXTENDED_DWELL);
>   if (ic->ic_des_esslen == 0)
>   req->scan_flags |= htole32(IWM_LMAC_SCAN_FLAG_PASSIVE);
> - else
> - req->scan_flags |=
> - htole32(IWM_LMAC_SCAN_FLAG_PRE_CONNECTION);
>   if (isset(sc->sc_enabled_capa, 
>   IWM_UCODE_TLV_CAPA_DS_PARAM_SET_IE_SUPPORT))
>   req->scan_flags |= htole32(IWM_LMAC_SCAN_FLAGS_RRM_ENABLED);
> @@ -5098,8 +5095,6 @@ iwm_umac_scan(struct iwm_softc *sc, int 
>   tail->direct_scan[0].len = ic->ic_des_esslen;
>   memcpy(tail->direct_scan[0].ssid, ic->ic_des_essid,
>   ic->ic_des_esslen);
> - req->general_flags |=
> - htole32(IWM_UMAC_SCAN_GEN_FLAGS_PRE_CONNECT);
>   } else
>   req->general_flags |= htole32(IWM_UMAC_SCAN_GEN_FLAGS_PASSIVE);
>  
> 

Re: [Patch] mg(1) tutorial: no columns :(

[Brian Callahan]

On 05/25/18 10:25, Leonid Bobrov wrote:

Mentioning would be nice.

display.c's 1.39 revision log says "off by default
to not kill slow serial lines" -_-



Then why not write that diff?
Something like the below, maybe? I'm not totally sold that we need it, but am 
not unsold either. I could go either way, depending on what others think.

~Brian

Index: tutorial
===
RCS file: /cvs/src/usr.bin/mg/tutorial,v
retrieving revision 1.17
diff -u -p -r1.17 tutorial
--- tutorial    30 May 2017 07:11:40 -    1.17
+++ tutorial    25 May 2018 18:34:32 -
@@ -218,10 +218,11 @@ you're editing a file named "tutorial".
 saved it, it should have a "**" to the left of those words. If this file is
 read-only, you should see a "%%" to the left of those words.

-To the right of the status line, you should see L followed by digits and C
-followed by some more digits. These indicate the line number and column number
-of the file that your cursor is currently on. If you move the cursor around,
-you should see the line and column number change.
+To the right of the status line, you should see L followed by digits and, if
+column-number-mode is enabled, C followed by some more digits. These indicate
+the line number and column number of the file that your cursor is currently
+on. If you move the cursor around, you should see the line and column number
+change.

 In the middle of the screen, you should see the word "(fundamental)" which
 indicates that the current editing mode is "fundamental-mode". The mg editor

Re: [patch] Skip background scan if bssid is set

[Stefan Sperling]

On Sun, Apr 29, 2018 at 03:39:07AM +0200, Jesper Wallin wrote:
> Hi all,
> 
> I recently learned that my AP behaves badly and I have packet loss when
> the background scan is running.

Hi Jesper,

Can you please check if this diff makes your AP work without
hardcoding its bssid?

Index: if_iwm.c
===
RCS file: /cvs/src/sys/dev/pci/if_iwm.c,v
retrieving revision 1.230
diff -u -p -r1.230 if_iwm.c
--- if_iwm.c23 May 2018 17:49:20 -  1.230
+++ if_iwm.c25 May 2018 17:32:41 -
@@ -4719,7 +4719,7 @@ iwm_lmac_scan_fill_channels(struct iwm_s
chan->iter_count = htole16(1);
chan->iter_interval = 0;
chan->flags = htole32(IWM_UNIFIED_SCAN_CHANNEL_PARTIAL);
-   if (n_ssids != 0 && !bgscan)
+   if (n_ssids != 0)
chan->flags |= htole32(1 << 1); /* select SSID 0 */
chan++;
nchan++;
@@ -4746,7 +4746,7 @@ iwm_umac_scan_fill_channels(struct iwm_s
chan->channel_num = ieee80211_mhz2ieee(c->ic_freq, 0);
chan->iter_count = 1;
chan->iter_interval = htole16(0);
-   if (n_ssids != 0 && !bgscan)
+   if (n_ssids != 0)
chan->flags = htole32(1 << 0); /* select SSID 0 */
chan++;
nchan++;
@@ -4898,9 +4898,6 @@ iwm_lmac_scan(struct iwm_softc *sc, int 
IWM_LMAC_SCAN_FLAG_EXTENDED_DWELL);
if (ic->ic_des_esslen == 0)
req->scan_flags |= htole32(IWM_LMAC_SCAN_FLAG_PASSIVE);
-   else
-   req->scan_flags |=
-   htole32(IWM_LMAC_SCAN_FLAG_PRE_CONNECTION);
if (isset(sc->sc_enabled_capa, 
IWM_UCODE_TLV_CAPA_DS_PARAM_SET_IE_SUPPORT))
req->scan_flags |= htole32(IWM_LMAC_SCAN_FLAGS_RRM_ENABLED);
@@ -5098,8 +5095,6 @@ iwm_umac_scan(struct iwm_softc *sc, int 
tail->direct_scan[0].len = ic->ic_des_esslen;
memcpy(tail->direct_scan[0].ssid, ic->ic_des_essid,
ic->ic_des_esslen);
-   req->general_flags |=
-   htole32(IWM_UMAC_SCAN_GEN_FLAGS_PRE_CONNECT);
} else
req->general_flags |= htole32(IWM_UMAC_SCAN_GEN_FLAGS_PASSIVE);
 

Re: [Patch] mg(1) tutorial: no columns :(

[Leonid Bobrov]

Mentioning would be nice.

display.c's 1.39 revision log says "off by default
to not kill slow serial lines" -_-

Re: [Patch] mg(1) tutorial: no columns :(

[Andras Farkas]

On Fri, May 25, 2018 at 7:43 AM, Leonid Bobrov  wrote:
> Index: tutorial
> ===
> RCS file: /cvs/src/usr.bin/mg/tutorial,v
> retrieving revision 1.17
> diff -u -p -u -p -r1.17 tutorial
> --- tutorial30 May 2017 07:11:40 -  1.17
> +++ tutorial25 May 2018 03:42:23 -
> @@ -218,10 +218,9 @@ you're editing a file named "tutorial".
>  saved it, it should have a "**" to the left of those words. If this file is
>  read-only, you should see a "%%" to the left of those words.
>
> -To the right of the status line, you should see L followed by digits and C
> -followed by some more digits. These indicate the line number and column 
> number
> -of the file that your cursor is currently on. If you move the cursor around,
> -you should see the line and column number change.
> +To the right of the status line, you should see L followed by digits. This
> +indicates the line number of the file that your cursor is currently on. If
> +you move the cursor up or down, you should see the line number change.
>
>  In the middle of the screen, you should see the word "(fundamental)" which
>  indicates that the current editing mode is "fundamental-mode". The mg editor
>
If you M-x column-number-mode you can enable seeing the column number
in mg.  Perhaps the tutorial should mention that somewhere, in
addition to, or instead?
Though having column-number-mode enabled by default might be nice, as
line-number-mode is enabled already.

[Patch] mg(1) tutorial: no columns :(

[Leonid Bobrov]

Index: tutorial
===
RCS file: /cvs/src/usr.bin/mg/tutorial,v
retrieving revision 1.17
diff -u -p -u -p -r1.17 tutorial
--- tutorial30 May 2017 07:11:40 -  1.17
+++ tutorial25 May 2018 03:42:23 -
@@ -218,10 +218,9 @@ you're editing a file named "tutorial". 
 saved it, it should have a "**" to the left of those words. If this file is 
 read-only, you should see a "%%" to the left of those words. 
 
-To the right of the status line, you should see L followed by digits and C
-followed by some more digits. These indicate the line number and column number
-of the file that your cursor is currently on. If you move the cursor around,
-you should see the line and column number change.
+To the right of the status line, you should see L followed by digits. This
+indicates the line number of the file that your cursor is currently on. If
+you move the cursor up or down, you should see the line number change.
 
 In the middle of the screen, you should see the word "(fundamental)" which 
 indicates that the current editing mode is "fundamental-mode". The mg editor

Re: Unlock 16 network-related syscalls

[Hrvoje Popovski]

On 25.5.2018. 10:35, Martin Pieuchot wrote:
> Updated diff that should prevent reported hangs, as analyzed by tb@ and
> visa@.


with this diff i can't reproduce lockup ..

tnx ..

Re: [UPDATE] FreeType 2.9.1

[Bryan Linton]

On 2018-05-20 14:15:47, Matthieu Herrb  wrote:
> On Thu, Apr 26, 2018 at 01:01:33PM +0200, David Coppa wrote:
> > 
> > Hi all,
> > 
> > Attached you will find an update to the latest freetype (2.9.1).
> > 
>
> ok matthieu@.
> 
> note that the uninialized memory access with some fonts reported by
> Brian Lyton is supposed to be fixed in this release (and I can't
> reproduce it anymore).
> 
> So you may want to revert the change in src/base/ftutil.c rev 1.10.
> 

Hello,

I was the original reporter of the above bug.  I can confirm that
everything works fine on the following snapshot:

OpenBSD 6.3-current (GENERIC.MP) #44: Tue May 22 22:13:37 MDT 2018

Thank you again to all who helped resolve it.

-- 
Bryan

Re: Unlock 16 network-related syscalls

[Mathieu -]

Theo Buehler wrote:
> On Fri, May 25, 2018 at 10:35:35AM +0200, Martin Pieuchot wrote:
> > On 22/05/18(Tue) 15:39, Martin Pieuchot wrote:
> > > By assuming that `f_data' is immutable, which AFAIK is true for sockets,
> > > we can remove the KERNEL_LOCK() from the following syscalls iff files are
> > > refcounted in an MP-safe way.
> > > 
> > > This diff includes the EBUSY check in dup2(2) which is currently required
> > > to avoid races with accept(2) and will later make our life easier wrt
> > > open(2).
> > > 
> > > It also includes the fdinsert() diff I sent earlier.
> > > 
> > > On top of that I'm introducing a global mutex, `fhdlk', that protects
> > > `f_count' and the implicit reference in `filehead'.
> > > 
> > > A socket stays alive as long as its associated file has a positive
> > > refcount.  When this refcount drops, fdrop() will be called and soclose()
> > > will free/clean `f_data'.   That's the only place where `f_data' is
> > > changed during the life of a socket.  That's why it is safe to dereference
> > > `f_data' when getsock() returned a valid & refcounted `fp'.
> > > 
> > > Many ktrace(2) internals now need to grab the KERNEL_LOCK(), just like 
> > > ptsignal().
> > > 
> > > Note that for unix, routing and pfkey sockets, solock() still grabs the
> > > KERNEL_LOCK().  So even if syscalls are marked as SY_NOLOCK that doesn't
> > > mean they won't grab it.  In fact some network functions like
> > > ifa_ifwithaddr() below now need to grab the KERNEL_LOCK().  That's good
> > > that means we're pushing the lock down.
> > > 
> > > Tests?  Comments?
> > 
> > Updated diff that should prevent reported hangs, as analyzed by tb@ and
> > visa@.
> 
> I've been running this on my main laptop since Visa figured out the
> problem on Wednesday and saw no more issues since then. I've completed
> a make build with a WITNESS kernel while letting youtube play videos.
> 

Hello,

Same here, been running with this since last night and I can't reproduce
the hang despite hitting hard on it. (The hang would happen in 10
seconds after starting chrome on my machine).

Mathieu-

Re: Unlock 16 network-related syscalls

[Theo Buehler]

On Fri, May 25, 2018 at 10:35:35AM +0200, Martin Pieuchot wrote:
> On 22/05/18(Tue) 15:39, Martin Pieuchot wrote:
> > By assuming that `f_data' is immutable, which AFAIK is true for sockets,
> > we can remove the KERNEL_LOCK() from the following syscalls iff files are
> > refcounted in an MP-safe way.
> > 
> > This diff includes the EBUSY check in dup2(2) which is currently required
> > to avoid races with accept(2) and will later make our life easier wrt
> > open(2).
> > 
> > It also includes the fdinsert() diff I sent earlier.
> > 
> > On top of that I'm introducing a global mutex, `fhdlk', that protects
> > `f_count' and the implicit reference in `filehead'.
> > 
> > A socket stays alive as long as its associated file has a positive
> > refcount.  When this refcount drops, fdrop() will be called and soclose()
> > will free/clean `f_data'.   That's the only place where `f_data' is
> > changed during the life of a socket.  That's why it is safe to dereference
> > `f_data' when getsock() returned a valid & refcounted `fp'.
> > 
> > Many ktrace(2) internals now need to grab the KERNEL_LOCK(), just like 
> > ptsignal().
> > 
> > Note that for unix, routing and pfkey sockets, solock() still grabs the
> > KERNEL_LOCK().  So even if syscalls are marked as SY_NOLOCK that doesn't
> > mean they won't grab it.  In fact some network functions like
> > ifa_ifwithaddr() below now need to grab the KERNEL_LOCK().  That's good
> > that means we're pushing the lock down.
> > 
> > Tests?  Comments?
> 
> Updated diff that should prevent reported hangs, as analyzed by tb@ and
> visa@.

I've been running this on my main laptop since Visa figured out the
problem on Wednesday and saw no more issues since then. I've completed
a make build with a WITNESS kernel while letting youtube play videos.

For people who want to do 'make build', the following additional diff is
needed:

Index: usr.sbin/pstat/pstat.8
===
RCS file: /var/cvs/src/usr.sbin/pstat/pstat.8,v
retrieving revision 1.52
diff -u -p -r1.52 pstat.8
--- usr.sbin/pstat/pstat.8  26 Nov 2016 11:18:43 -  1.52
+++ usr.sbin/pstat/pstat.8  23 May 2018 16:33:54 -
@@ -101,8 +101,6 @@ open for appending
 exclusive or shared lock present
 .It I
 signal pgrp when data ready
-.It l
-file descriptor slot is larval
 .El
 .It CNT
 Number of processes that know this open file.
Index: usr.sbin/pstat/pstat.c
===
RCS file: /var/cvs/src/usr.sbin/pstat/pstat.c,v
retrieving revision 1.114
diff -u -p -r1.114 pstat.c
--- usr.sbin/pstat/pstat.c  2 Jan 2018 06:38:45 -   1.114
+++ usr.sbin/pstat/pstat.c  23 May 2018 16:22:56 -
@@ -1044,8 +1044,6 @@ filemode(void)
 
if (kf->f_iflags & FIF_HASLOCK)
*fbp++ = 'L';
-   if (kf->f_iflags & FIF_LARVAL)
-   *fbp++ = 'l';
 
*fbp = '\0';
(void)printf("%6s  %3ld", flagbuf, (long)kf->f_count);

Re: Unlock 16 network-related syscalls

[Martin Pieuchot]

On 22/05/18(Tue) 15:39, Martin Pieuchot wrote:
> By assuming that `f_data' is immutable, which AFAIK is true for sockets,
> we can remove the KERNEL_LOCK() from the following syscalls iff files are
> refcounted in an MP-safe way.
> 
> This diff includes the EBUSY check in dup2(2) which is currently required
> to avoid races with accept(2) and will later make our life easier wrt
> open(2).
> 
> It also includes the fdinsert() diff I sent earlier.
> 
> On top of that I'm introducing a global mutex, `fhdlk', that protects
> `f_count' and the implicit reference in `filehead'.
> 
> A socket stays alive as long as its associated file has a positive
> refcount.  When this refcount drops, fdrop() will be called and soclose()
> will free/clean `f_data'.   That's the only place where `f_data' is
> changed during the life of a socket.  That's why it is safe to dereference
> `f_data' when getsock() returned a valid & refcounted `fp'.
> 
> Many ktrace(2) internals now need to grab the KERNEL_LOCK(), just like 
> ptsignal().
> 
> Note that for unix, routing and pfkey sockets, solock() still grabs the
> KERNEL_LOCK().  So even if syscalls are marked as SY_NOLOCK that doesn't
> mean they won't grab it.  In fact some network functions like
> ifa_ifwithaddr() below now need to grab the KERNEL_LOCK().  That's good
> that means we're pushing the lock down.
> 
> Tests?  Comments?

Updated diff that should prevent reported hangs, as analyzed by tb@ and
visa@.

Index: kern/exec_script.c
===
RCS file: /cvs/src/sys/kern/exec_script.c,v
retrieving revision 1.44
diff -u -p -r1.44 exec_script.c
--- kern/exec_script.c  2 May 2018 02:24:56 -   1.44
+++ kern/exec_script.c  25 May 2018 08:24:33 -
@@ -170,17 +170,20 @@ check_shell:
 #endif
 
fdplock(p->p_fd);
-   error = falloc(p, 0, , >ep_fd);
-   fdpunlock(p->p_fd);
-   if (error)
+   error = falloc(p, , >ep_fd);
+   if (error) {
+   fdpunlock(p->p_fd);
goto fail;
+   }
 
epp->ep_flags |= EXEC_HASFD;
fp->f_type = DTYPE_VNODE;
fp->f_ops = 
fp->f_data = (caddr_t) scriptvp;
fp->f_flag = FREAD;
-   FILE_SET_MATURE(fp, p);
+   fdinsert(p->p_fd, epp->ep_fd, 0, fp);
+   fdpunlock(p->p_fd);
+   FRELE(fp, p);
}
 
/* set up the parameters for the recursive check_exec() call */
Index: kern/init_sysent.c
===
RCS file: /cvs/src/sys/kern/init_sysent.c,v
retrieving revision 1.191
diff -u -p -r1.191 init_sysent.c
--- kern/init_sysent.c  12 Dec 2017 01:13:14 -  1.191
+++ kern/init_sysent.c  25 May 2018 08:24:33 -
@@ -1,4 +1,4 @@
-/* $OpenBSD: init_sysent.c,v 1.191 2017/12/12 01:13:14 deraadt Exp $   
*/
+/* $OpenBSD$   */
 
 /*
  * System call switch table.
@@ -76,17 +76,17 @@ struct sysent sysent[] = {
{ 0, 0, 0,
sys_nosys },/* 26 = unimplemented ptrace */
 #endif
-   { 3, s(struct sys_recvmsg_args), 0,
+   { 3, s(struct sys_recvmsg_args), SY_NOLOCK | 0,
sys_recvmsg },  /* 27 = recvmsg */
-   { 3, s(struct sys_sendmsg_args), 0,
+   { 3, s(struct sys_sendmsg_args), SY_NOLOCK | 0,
sys_sendmsg },  /* 28 = sendmsg */
-   { 6, s(struct sys_recvfrom_args), 0,
+   { 6, s(struct sys_recvfrom_args), SY_NOLOCK | 0,
sys_recvfrom }, /* 29 = recvfrom */
-   { 3, s(struct sys_accept_args), 0,
+   { 3, s(struct sys_accept_args), SY_NOLOCK | 0,
sys_accept },   /* 30 = accept */
-   { 3, s(struct sys_getpeername_args), 0,
+   { 3, s(struct sys_getpeername_args), SY_NOLOCK | 0,
sys_getpeername },  /* 31 = getpeername */
-   { 3, s(struct sys_getsockname_args), 0,
+   { 3, s(struct sys_getsockname_args), SY_NOLOCK | 0,
sys_getsockname },  /* 32 = getsockname */
{ 2, s(struct sys_access_args), 0,
sys_access },   /* 33 = access */
@@ -218,7 +218,7 @@ struct sysent sysent[] = {
sys_nanosleep },/* 91 = nanosleep */
{ 3, s(struct sys_fcntl_args), 0,
sys_fcntl },/* 92 = fcntl */
-   { 4, s(struct sys_accept4_args), 0,
+   { 4, s(struct sys_accept4_args), SY_NOLOCK | 0,
sys_accept4 },  /* 93 = accept4 */
{ 5, s(struct sys___thrsleep_args), 0,
sys___thrsleep },   /* 94 = __thrsleep */
@@ -226,9 +226,9 @@ struct sysent sysent[] = {
sys_fsync },/* 95 = fsync */
{ 3, s(struct 

in_ioctl: split out SIOC{A,D,S}IFADDR

[Theo Buehler]

The second step of merging the two switches in in_ioctl() is to split
the six ioctls in this function into two families of three and to start
disentangling them. While it is not strictly needed, it seemed cleanest
to introduce a new function analogous to what was done in in6_ioctl().
This way we have a clear separation of the two families and we can
manipulate them independently. We can easily merge them back together
later on if that is desired.

The diff below copies the boiler plate from in_ioctl() to a new function
and moves SIOCAIFADDR, SIOCDIFADDR, and SIOCSIFADDR there. The next step
will then be to actually merge the two cases per ioctl.

Index: sys/netinet/in.c
===
RCS file: /var/cvs/src/sys/netinet/in.c,v
retrieving revision 1.152
diff -u -p -r1.152 in.c
--- sys/netinet/in.c25 May 2018 06:05:08 -  1.152
+++ sys/netinet/in.c25 May 2018 06:24:30 -
@@ -84,6 +84,7 @@
 
 void in_socktrim(struct sockaddr_in *);
 
+int in_ioctl_change_ifaddr(u_long, caddr_t, struct ifnet *, int);
 int in_ioctl_get(u_long, caddr_t, struct ifnet *);
 void in_purgeaddr(struct ifaddr *);
 int in_addhost(struct in_ifaddr *, struct sockaddr_in *);
@@ -216,7 +217,6 @@ in_ioctl(u_long cmd, caddr_t data, struc
struct in_aliasreq *ifra = (struct in_aliasreq *)data;
struct sockaddr_in oldaddr;
int error = 0;
-   int newifaddr;
 
if (ifp == NULL)
return (ENXIO);
@@ -230,6 +230,7 @@ in_ioctl(u_long cmd, caddr_t data, struc
case SIOCAIFADDR:
case SIOCDIFADDR:
case SIOCSIFADDR:
+   return in_ioctl_change_ifaddr(cmd, data, ifp, privileged);
case SIOCSIFNETMASK:
case SIOCSIFDSTADDR:
case SIOCSIFBRDADDR:
@@ -248,47 +249,6 @@ in_ioctl(u_long cmd, caddr_t data, struc
}
 
switch (cmd) {
-   case SIOCAIFADDR:
-   case SIOCDIFADDR:
-   if (ifra->ifra_addr.sin_family == AF_INET) {
-   for (; ifa != NULL; ifa = TAILQ_NEXT(ifa, ifa_list)) {
-   if ((ifa->ifa_addr->sa_family == AF_INET) &&
-   ifatoia(ifa)->ia_addr.sin_addr.s_addr ==
-   ifra->ifra_addr.sin_addr.s_addr)
-   break;
-   }
-   ia = ifatoia(ifa);
-   }
-   if (cmd == SIOCDIFADDR && ia == NULL) {
-   error = EADDRNOTAVAIL;
-   goto err;
-   }
-   /* FALLTHROUGH */
-   case SIOCSIFADDR:
-   if (!privileged) {
-   error = EPERM;
-   goto err;
-   }
-
-   if (ia == NULL) {
-   ia = malloc(sizeof *ia, M_IFADDR, M_WAITOK | M_ZERO);
-   ia->ia_addr.sin_family = AF_INET;
-   ia->ia_addr.sin_len = sizeof(ia->ia_addr);
-   ia->ia_ifa.ifa_addr = sintosa(>ia_addr);
-   ia->ia_ifa.ifa_dstaddr = sintosa(>ia_dstaddr);
-   ia->ia_ifa.ifa_netmask = sintosa(>ia_sockmask);
-   ia->ia_sockmask.sin_len = 8;
-   if (ifp->if_flags & IFF_BROADCAST) {
-   ia->ia_broadaddr.sin_len = sizeof(ia->ia_addr);
-   ia->ia_broadaddr.sin_family = AF_INET;
-   }
-   ia->ia_ifp = ifp;
-
-   newifaddr = 1;
-   } else
-   newifaddr = 0;
-   break;
-
case SIOCSIFNETMASK:
case SIOCSIFDSTADDR:
case SIOCSIFBRDADDR:
@@ -342,7 +302,76 @@ in_ioctl(u_long cmd, caddr_t data, struc
ia->ia_netmask = ia->ia_sockmask.sin_addr.s_addr =
ifra->ifra_addr.sin_addr.s_addr;
break;
+   }
+
+err:
+   NET_UNLOCK();
+   return (error);
+}
+
+int
+in_ioctl_change_ifaddr(u_long cmd, caddr_t data, struct ifnet *ifp,
+int privileged)
+{
+   struct ifreq *ifr = (struct ifreq *)data;
+   struct ifaddr *ifa;
+   struct in_ifaddr *ia = NULL;
+   struct in_aliasreq *ifra = (struct in_aliasreq *)data;
+   int error = 0;
+   int newifaddr;
 
+   NET_LOCK();
+
+   TAILQ_FOREACH(ifa, >if_addrlist, ifa_list) {
+   if (ifa->ifa_addr->sa_family == AF_INET) {
+   ia = ifatoia(ifa);
+   break;
+   }
+   }
+
+   switch (cmd) {
+   case SIOCAIFADDR:
+   case SIOCDIFADDR:
+   if (ifra->ifra_addr.sin_family == AF_INET) {
+   for (; ifa != NULL; ifa = TAILQ_NEXT(ifa, ifa_list)) {
+   if ((ifa->ifa_addr->sa_family == AF_INET) &&
+   ifatoia(ifa)->ia_addr.sin_addr.s_addr ==
+   

Re: smtpd.conf new grammar

[Gilles Chehade]

On Thu, May 24, 2018 at 04:38:17PM -0400, Rupert Gallagher wrote:
> On Thu, May 24, 2018 at 14:18, Gilles Chehade  wrote:
> 
> > In effect, instead of having:
> > accept from any for local deliver to mbox
> >
> > You will have:
> > action "my_action" mbox
> > match from any for local action "my_action"
> 
> It may solve some obscure technical problem, but is a horrible thing to read 
> and write. How about keeping the best of both worlds? Leave the old beautiful 
> PF-like syntax to humans, and translate it into the newEgyptian(tm) on the 
> fly?

It doesn't solve "obscure" technical problems, it solves _many_ issues a
lot of users have reported ranging from syntax ambiguity to envelopes on
disk not reflecting changes in configuration. One-line rules have lot of
consequences which go far beyond the configuration phase: the structures
are impacted, the ruleset evaluation is impacted, the aliases expansions
are impacted, the queue is impacted, format of envelopes are impacted, I
could go on and on since this impacts almost the entire daemon actually.


The impact is not just cosmethic stuff. I removed _hundreds_ of lines of
very unnecessary and complex code that was ONLY there to make the design
error work. Some of these removals led to concrete security improvements
like .forward files, written by untrusted users, be processed with their
privileges rather than _smtpd. Not doable with one-line rules.


I could write pages about the shortcomings from the previous config, and
pages about why it can't be made to work and why the new config fixes it
in the proper way. We tried hard to find ways to retain a one-line rules
configuration but after months we exhausted the ideas and we have a much
clearer understanding that it's NOT doable. Either we accept that, or we
are just going to grow more complex and slowly stop writing code because
every time you touch an area there's a risk you run into the complexity.


You don't have to trust my word:

> How about keeping the best of both worlds? Leave the old beautiful PF-like 
> syntax to humans,
> and translate it into the newEgyptian(tm) on the fly?

If this was possible, then you could easily write a translator that will
convert old grammar to new one without removing all the benefits and not
reintroducing the complex code.

By all means, show me, I'd be impressed for real.

-- 
Gilles Chehade

https://www.poolp.org  @poolpOrg