Re: ssh/sshd change in snaps
On Thu, 11 Nov 2021, Damien Miller wrote: > Hi, > > Snaps is now carrying a change to ssh/sshd that converts their > mainloops from select(2) to poll/ppoll(2). This change should be > completely transparent, but please be on the lookout for any weird > behaviour. Bugs in the revised mainloop are most likely to appear > as crashes, hangs or ssh/sshd using lots of CPU when idle. > > Please report anything you see here, to openssh@ or to me. FYI this has gone through one interation of bugfixes since it first went in. If you are running a snap from yesterday and experience problems then please upgrade to a newer one and retest. -d
X server updated to version 21.1.1
Hi, I've updated the X server in Xenocara to version 21.1.1, together with Freetype (2.11.0) and fontconfig 2.13.94. One of the visible change of this update is that some LCD panels now report their real physical dimensions to the X server, rather than just computing it based on a default resolution of 96dpi. This means that the actual DPI computed from these dimensions will change and impact the size of the displayed fonts. If your fonts have chanded sizes too much after the last snapshot update, you can add xrandr --dpi 96 in your ~/.xsession (or ~/.xinitrc) file to set the resolution manually to the previous default. You can also fiddle with the X resources (~/.Xresources ) to fix font sizes in invidual applications. Older X applications are more likely to mis-behave and need the global dpi fix. -- Matthieu Herrb
Re: iwx(4): fix TID array index bound checks
> Date: Fri, 12 Nov 2021 13:37:47 +0100
> From: Stefan Sperling
>
> This tid variable is used as an array index and must thus be smaller
> than MAX_TID_COUNT (which is 8).
>
> The variable will be set to 8 if the AP wants us to use TID 8 for Rx agg,
> which I've never seen happen in practice, though it is possible.
>
> Our driver uses this value as an index into an array of
> IEEE80211_NUM_TID (16) elements, and thus would not crash.
>
> But the value is passed on to firmware which does who knows what with it.
> The Linux driver uses this value as an index into an array ("tid_to_baid")
> of MAX_TID_COUNT elements. Perhaps firmware has a similar limitation?
> I cannot tell.
>
> ok?
ok kettenis@
> diff 0433237ad58131957ccba3ddd837ed772dbec416
> 6629860d5767033824e2fb1c0f621c18216a7316
> blob - 5cac83fd48ebe997a4cd7842730b0cb05f7c28ee
> blob + 6e67f7db864ec9a7ca61057b7284e7d7a6bffe5d
> --- sys/dev/pci/if_iwx.c
> +++ sys/dev/pci/if_iwx.c
> @@ -3264,7 +3264,7 @@ iwx_ampdu_rx_start(struct ieee80211com *ic, struct iee
> struct iwx_softc *sc = IC2IFP(ic)->if_softc;
>
> if (sc->sc_rx_ba_sessions >= IWX_MAX_RX_BA_SESSIONS ||
> - tid > IWX_MAX_TID_COUNT)
> + tid >= IWX_MAX_TID_COUNT)
> return ENOSPC;
>
> if (sc->ba_rx.start_tidmask & (1 << tid))
> @@ -3286,7 +3286,7 @@ iwx_ampdu_rx_stop(struct ieee80211com *ic, struct ieee
> {
> struct iwx_softc *sc = IC2IFP(ic)->if_softc;
>
> - if (tid > IWX_MAX_TID_COUNT || sc->ba_rx.stop_tidmask & (1 << tid))
> + if (tid >= IWX_MAX_TID_COUNT || sc->ba_rx.stop_tidmask & (1 << tid))
> return;
>
> sc->ba_rx.stop_tidmask = (1 << tid);
>
>
Re: iwx(4): fix Tx ring array size
> Date: Fri, 12 Nov 2021 13:22:06 +0100
> From: Stefan Sperling
>
> The iwx Tx ring array is one entry too short due to an off-by-one error.
>
> Fortunately, this bug is harmless. The last Tx agg queue is never used
> because ieee80211_classify() only returns TID values in the range 0 - 3.
> And iterations over the txq array use nitems() to find the upper bound.
>
> The possiblity of shrinking the txq array by 4 elements to get rid of
> unused Tx agg queues could be investigated later.
> For now, just fix the off-by-one error.
>
> ok?
Can't break anything.
ok kettenis@
> diff b19fd44c79660fc69d55de88265e00892505e7ab
> 0433237ad58131957ccba3ddd837ed772dbec416
> blob - e29148ff81ffa620b806f9244c7ede8f05fb417c
> blob + 4f310ad504e6f81a0ee65a11614dc5bd70f79fef
> --- sys/dev/pci/if_iwxreg.h
> +++ sys/dev/pci/if_iwxreg.h
> @@ -1420,6 +1420,7 @@ struct iwx_gen3_bc_tbl {
> #define IWX_MAX_TID_COUNT8
> #define IWX_FIRST_AGG_TX_QUEUE (IWX_DQA_MGMT_QUEUE + 1)
> #define IWX_LAST_AGG_TX_QUEUE(IWX_FIRST_AGG_TX_QUEUE +
> IWX_MAX_TID_COUNT - 1)
> +#define IWX_NUM_TX_QUEUES(IWX_LAST_AGG_TX_QUEUE + 1)
>
> /**
> * Max Tx window size is the max number of contiguous TFDs that the scheduler
> blob - 009848fc25471ad90717094d1f67d2f4d22a3d6a
> blob + 719d9b6295b9eb9c31b36a98bcb0259a8a2f4cf3
> --- sys/dev/pci/if_iwxvar.h
> +++ sys/dev/pci/if_iwxvar.h
> @@ -497,7 +497,7 @@ struct iwx_softc {
> int sc_msix;
>
> /* TX/RX rings. */
> - struct iwx_tx_ring txq[IWX_LAST_AGG_TX_QUEUE];
> + struct iwx_tx_ring txq[IWX_NUM_TX_QUEUES];
> struct iwx_rx_ring rxq;
> int qfullmsk;
> int qenablemsk;
>
>
support probe as variable in btrace
This is something I missed to do easy btrace check like:
syscall:exit:entry,
syscall:fork:entry,
syscall:sigaction:entry,
syscall:execve:entry,
syscall:open:entry { @[probe] = count(); }
This will produce something like this as output:
@[syscall:open:entry]: 844
@[syscall:sigaction:entry]: 480
@[syscall:execve:entry]: 27
@[syscall:exit:entry]: 26
@[syscall:fork:entry]: 24
Works for me. I decided to qsort the dt_dtpis array so that bsearch can be
used for quick lookups by id. If the kernel ensures that the returned
array from the ioctl is always sorted this could be skipped.
--
:wq Claudio
Index: bt.5
===
RCS file: /cvs/src/usr.sbin/btrace/bt.5,v
retrieving revision 1.12
diff -u -p -r1.12 bt.5
--- bt.53 Oct 2021 22:01:48 - 1.12
+++ bt.512 Nov 2021 14:05:16 -
@@ -105,6 +105,8 @@ Kernel stack of the current thread.
Timestamp of the event in nanoseconds.
.It Va pid
Process ID of the current thread.
+.It Va probe
+Full name of the probe.
.It Va retval
Return value of the traced syscall.
.It Va tid
Index: bt_parse.y
===
RCS file: /cvs/src/usr.sbin/btrace/bt_parse.y,v
retrieving revision 1.44
diff -u -p -r1.44 bt_parse.y
--- bt_parse.y 3 Oct 2021 22:01:48 - 1.44
+++ bt_parse.y 12 Nov 2021 13:46:22 -
@@ -717,6 +717,7 @@ lookup(char *s)
{ "pid",BUILTIN,B_AT_BI_PID },
{ "print", F_PRINT,B_AC_PRINT },
{ "printf", FUNCN, B_AC_PRINTF },
+ { "probe", BUILTIN,B_AT_BI_PROBE },
{ "retval", BUILTIN,B_AT_BI_RETVAL },
{ "str",STR,B_AT_FN_STR },
{ "sum",MOP1, B_AT_MF_SUM },
Index: bt_parser.h
===
RCS file: /cvs/src/usr.sbin/btrace/bt_parser.h,v
retrieving revision 1.21
diff -u -p -r1.21 bt_parser.h
--- bt_parser.h 3 Oct 2021 22:01:48 - 1.21
+++ bt_parser.h 12 Nov 2021 13:12:43 -
@@ -147,6 +147,7 @@ struct bt_arg {
B_AT_BI_ARG9,
B_AT_BI_ARGS,
B_AT_BI_RETVAL,
+ B_AT_BI_PROBE,
B_AT_FN_STR,/* str($1); str($1, 3); */
Index: btrace.c
===
RCS file: /cvs/src/usr.sbin/btrace/btrace.c,v
retrieving revision 1.59
diff -u -p -r1.59 btrace.c
--- btrace.c24 Oct 2021 14:18:58 - 1.59
+++ btrace.c12 Nov 2021 13:55:52 -
@@ -256,6 +256,18 @@ read_btfile(const char *filename, size_t
return fcontent;
}
+static int
+dtpi_cmp(const void *a, const void *b)
+{
+ const struct dtioc_probe_info *ai = a, *bi = b;
+
+ if (ai->dtpi_pbn > bi->dtpi_pbn)
+ return 1;
+ if (ai->dtpi_pbn < bi->dtpi_pbn)
+ return -1;
+ return 0;
+}
+
void
dtpi_cache(int fd)
{
@@ -276,6 +288,8 @@ dtpi_cache(int fd)
dtpr.dtpr_probes = dt_dtpis;
if (ioctl(fd, DTIOCGPLIST, ))
err(1, "DTIOCGPLIST");
+
+ qsort(dt_dtpis, dt_ndtpi, sizeof(*dt_dtpis), dtpi_cmp);
}
void
@@ -351,6 +365,15 @@ dtpi_get_by_value(const char *prov, cons
return NULL;
}
+static struct dtioc_probe_info *
+dtpi_get_by_id(unsigned int pbn)
+{
+ struct dtioc_probe_info d;
+
+ d.dtpi_pbn = pbn;
+ return bsearch(, dt_dtpis, dt_ndtpi, sizeof(*dt_dtpis), dtpi_cmp);
+}
+
void
rules_do(int fd)
{
@@ -1263,6 +1286,8 @@ ba_name(struct bt_arg *ba)
return "args";
case B_AT_BI_RETVAL:
return "retval";
+ case B_AT_BI_PROBE:
+ return "probe";
case B_AT_FN_STR:
return "str";
case B_AT_OP_PLUS:
@@ -1372,6 +1397,9 @@ ba2long(struct bt_arg *ba, struct dt_evt
case B_AT_BI_RETVAL:
val = dtev->dtev_retval[0];
break;
+ case B_AT_BI_PROBE:
+ val = dtev->dtev_pbn;
+ break;
case B_AT_OP_PLUS ... B_AT_OP_LOR:
val = baexpr2long(ba, dtev);
break;
@@ -1390,6 +1418,7 @@ ba2str(struct bt_arg *ba, struct dt_evt
{
static char buf[STRLEN];
struct bt_var *bv;
+ struct dtioc_probe_info *dtpi;
const char *str;
buf[0] = '\0';
@@ -1436,6 +1465,15 @@ ba2str(struct bt_arg *ba, struct dt_evt
snprintf(buf, sizeof(buf), "%ld", (long)dtev->dtev_retval[0]);
str = buf;
break;
+ case B_AT_BI_PROBE:
+ dtpi = dtpi_get_by_id(dtev->dtev_pbn);
+ if (dtpi != NULL)
+ snprintf(buf, sizeof(buf), "%s:%s:%s",
+ dtpi->dtpi_prov, dtpi_func(dtpi), dtpi->dtpi_name);
+ else
+
Re: make iwx(4) use per-queue Tx timers
On Mon, Nov 08, 2021 at 10:59:55AM +0100, Stefan Sperling wrote:
> iwx(4) has an issue which occurs very occasionally for me (every couple
> of days or sometimes even weeks) where ssh(1) fails to connect until the
> interface is reset with ifconfig down/up.
>
> The initial protocol exchange with the SSH server succeeds, but as soon
> as the client enters interactive state and sets TCP_NODELAY the connection
> will hang.
> While the interface is in this broken state other traffic such as ping
> still makes it through, but any new ssh connection attempts show the
> same symptom. It is just the interactive SSH packets which are stuck.
>
> TCP_NODELAY packets are mapped to a special QoS TID by net80211, and each
> such TID gets mapped to a dedicated Tx aggregation queue by the driver.
> Combined with the observation that the Linux driver carries a workaround
> for "stuck" individual Tx queues, we are likely facing a situation where
> only one Tx queue in the device has stopped working.
>
> Our interface watchdog cannot detect single Tx queue failures at present.
> It is satisfied as long as packets make it out on any queue.
> With the patch below we use a Tx timer per queue which will hopefully result
> in a device timeout when the problem occurs. A manual reset will no longer be
> required to unwedge things since the watchdog should now trigger a reset.
>
> It is possible that other hangs which have been reported are related to
> this but show different symptoms depending on which Tx queue gets stuck.
> If the default Tx agg queue gets stuck it will become impossible to send
> data packets that are not marked as TCP_NODELAY. If you are connected over
> ssh while this happens the open ssh session might prevent the watchdog from
> triggering and no new connections can be made.
>
> I wish we had a fix for Tx queues getting stuck in the first place but this
> is the best I can do.
>
> ok?
Did anyone try this yet? It is working well for me.
This new version of the patch has been rebased on top of two small
iwx(4) driver fixes I have sent out separately today. With this,
the size of the sc_tx_timer array is corrected to match the new
size of the Tx ring array.
Apply them all in sequence:
"fix Tx ring array size"
https://marc.info/?l=openbsd-tech=163671980016530=2
"fix TID array index bound checks"
https://marc.info/?l=openbsd-tech=163672074217035=2
ok?
diff 6629860d5767033824e2fb1c0f621c18216a7316
fbfd8a2c2e804554680d3879819c789931916dc5
blob - 6e67f7db864ec9a7ca61057b7284e7d7a6bffe5d
blob + e1793a5b742f691cbffa930e8f6363538190d590
--- sys/dev/pci/if_iwx.c
+++ sys/dev/pci/if_iwx.c
@@ -4552,8 +4552,6 @@ iwx_rx_tx_cmd(struct iwx_softc *sc, struct iwx_rx_pack
bus_dmamap_sync(sc->sc_dmat, data->map, 0, IWX_RBUF_SIZE,
BUS_DMASYNC_POSTREAD);
- sc->sc_tx_timer = 0;
-
/* Sanity checks. */
if (sizeof(*tx_resp) > len)
return;
@@ -4563,6 +4561,8 @@ iwx_rx_tx_cmd(struct iwx_softc *sc, struct iwx_rx_pack
tx_resp->frame_count * sizeof(tx_resp->status) > len)
return;
+ sc->sc_tx_timer[qid] = 0;
+
if (tx_resp->frame_count > 1) /* A-MPDU */
return;
@@ -4658,7 +4658,7 @@ iwx_rx_compressed_ba(struct iwx_softc *sc, struct iwx_
idx = le16toh(ba_tfd->tfd_index);
if (idx >= IWX_TX_RING_COUNT)
continue;
- sc->sc_tx_timer = 0;
+ sc->sc_tx_timer[qid] = 0;
iwx_txq_advance(sc, ring, idx);
iwx_clear_oactive(sc, ring);
}
@@ -5433,6 +5433,9 @@ iwx_tx(struct iwx_softc *sc, struct mbuf *m, struct ie
sc->qfullmsk |= 1 << ring->qid;
}
+ if (ic->ic_if.if_flags & IFF_UP)
+ sc->sc_tx_timer[ring->qid] = 15;
+
return 0;
}
@@ -7973,10 +7976,8 @@ iwx_start(struct ifnet *ifp)
continue;
}
- if (ifp->if_flags & IFF_UP) {
- sc->sc_tx_timer = 15;
+ if (ifp->if_flags & IFF_UP)
ifp->if_timer = 1;
- }
}
return;
@@ -8045,7 +8046,8 @@ iwx_stop(struct ifnet *ifp)
struct iwx_rxba_data *rxba = >sc_rxba_data[i];
iwx_clear_reorder_buffer(sc, rxba);
}
- ifp->if_timer = sc->sc_tx_timer = 0;
+ memset(sc->sc_tx_timer, 0, sizeof(sc->sc_tx_timer));
+ ifp->if_timer = 0;
splx(s);
}
@@ -8054,21 +8056,30 @@ void
iwx_watchdog(struct ifnet *ifp)
{
struct iwx_softc *sc = ifp->if_softc;
+ int i;
ifp->if_timer = 0;
- if (sc->sc_tx_timer > 0) {
- if (--sc->sc_tx_timer == 0) {
- printf("%s: device timeout\n", DEVNAME(sc));
- if (ifp->if_flags & IFF_DEBUG) {
- iwx_nic_error(sc);
- iwx_dump_driver_status(sc);
+
+
iwx(4): fix TID array index bound checks
This tid variable is used as an array index and must thus be smaller
than MAX_TID_COUNT (which is 8).
The variable will be set to 8 if the AP wants us to use TID 8 for Rx agg,
which I've never seen happen in practice, though it is possible.
Our driver uses this value as an index into an array of
IEEE80211_NUM_TID (16) elements, and thus would not crash.
But the value is passed on to firmware which does who knows what with it.
The Linux driver uses this value as an index into an array ("tid_to_baid")
of MAX_TID_COUNT elements. Perhaps firmware has a similar limitation?
I cannot tell.
ok?
diff 0433237ad58131957ccba3ddd837ed772dbec416
6629860d5767033824e2fb1c0f621c18216a7316
blob - 5cac83fd48ebe997a4cd7842730b0cb05f7c28ee
blob + 6e67f7db864ec9a7ca61057b7284e7d7a6bffe5d
--- sys/dev/pci/if_iwx.c
+++ sys/dev/pci/if_iwx.c
@@ -3264,7 +3264,7 @@ iwx_ampdu_rx_start(struct ieee80211com *ic, struct iee
struct iwx_softc *sc = IC2IFP(ic)->if_softc;
if (sc->sc_rx_ba_sessions >= IWX_MAX_RX_BA_SESSIONS ||
- tid > IWX_MAX_TID_COUNT)
+ tid >= IWX_MAX_TID_COUNT)
return ENOSPC;
if (sc->ba_rx.start_tidmask & (1 << tid))
@@ -3286,7 +3286,7 @@ iwx_ampdu_rx_stop(struct ieee80211com *ic, struct ieee
{
struct iwx_softc *sc = IC2IFP(ic)->if_softc;
- if (tid > IWX_MAX_TID_COUNT || sc->ba_rx.stop_tidmask & (1 << tid))
+ if (tid >= IWX_MAX_TID_COUNT || sc->ba_rx.stop_tidmask & (1 << tid))
return;
sc->ba_rx.stop_tidmask = (1 << tid);
iwx(4): fix Tx ring array size
The iwx Tx ring array is one entry too short due to an off-by-one error.
Fortunately, this bug is harmless. The last Tx agg queue is never used
because ieee80211_classify() only returns TID values in the range 0 - 3.
And iterations over the txq array use nitems() to find the upper bound.
The possiblity of shrinking the txq array by 4 elements to get rid of
unused Tx agg queues could be investigated later.
For now, just fix the off-by-one error.
ok?
diff b19fd44c79660fc69d55de88265e00892505e7ab
0433237ad58131957ccba3ddd837ed772dbec416
blob - e29148ff81ffa620b806f9244c7ede8f05fb417c
blob + 4f310ad504e6f81a0ee65a11614dc5bd70f79fef
--- sys/dev/pci/if_iwxreg.h
+++ sys/dev/pci/if_iwxreg.h
@@ -1420,6 +1420,7 @@ struct iwx_gen3_bc_tbl {
#define IWX_MAX_TID_COUNT 8
#define IWX_FIRST_AGG_TX_QUEUE (IWX_DQA_MGMT_QUEUE + 1)
#define IWX_LAST_AGG_TX_QUEUE (IWX_FIRST_AGG_TX_QUEUE + IWX_MAX_TID_COUNT - 1)
+#define IWX_NUM_TX_QUEUES (IWX_LAST_AGG_TX_QUEUE + 1)
/**
* Max Tx window size is the max number of contiguous TFDs that the scheduler
blob - 009848fc25471ad90717094d1f67d2f4d22a3d6a
blob + 719d9b6295b9eb9c31b36a98bcb0259a8a2f4cf3
--- sys/dev/pci/if_iwxvar.h
+++ sys/dev/pci/if_iwxvar.h
@@ -497,7 +497,7 @@ struct iwx_softc {
int sc_msix;
/* TX/RX rings. */
- struct iwx_tx_ring txq[IWX_LAST_AGG_TX_QUEUE];
+ struct iwx_tx_ring txq[IWX_NUM_TX_QUEUES];
struct iwx_rx_ring rxq;
int qfullmsk;
int qenablemsk;
Re: uhidev: claim multiple report ids [3/N]
On Thu, Nov 11, 2021 at 05:09:35PM -0800, Greg Steuck wrote:
> Anton Lindqvist writes:
>
> > On Thu, Nov 11, 2021 at 03:29:15PM +0100, Anton Lindqvist wrote:
> >> Hi,
> >> The second attempt to solve the uhidev claim multiple report ids
> >> conflict didn't work out either as it broke fido(4). Signalling claim
> >> multiple report ids to the match routines using the report id does not
> >> work as all 256 values already have semantic meaning. I instead want to
> >> use `uha->claimed != NULL' to signal that multiple report ids can be
> >> claimed. Before doing so, refactor in order to make an upcoming diff
> >> with the actual fix significantly smaller.
> >>
> >> No intended^W functional change.
> >>
> >> Comments? OK?
> >
> > ... and here's the actual fix applied on top of the previous diff.
>
> The pair of diffs seems to work for me (fido remains operational unlike
> the previous iteration). There's a minor change in dmesg output which is
> not otherwise consequential:
Thanks, some drivers (ums and ukbd for instance) are still missing a
UHIDEV_CLAIM_MULTIPLE_REPORTID conditional. That can be fixed later on.
For now, keep using 255 as the report id when claiming multiple report
ids.
Here's a new iteration of the second diff. Another round of testing
would be much appreciated.
diff --git sys/dev/usb/uhidev.h sys/dev/usb/uhidev.h
index 86217fb8880..b8daf014662 100644
--- sys/dev/usb/uhidev.h
+++ sys/dev/usb/uhidev.h
@@ -75,12 +75,11 @@ struct uhidev_attach_arg {
struct usb_attach_arg *uaa;
struct uhidev_softc *parent;
uint8_t reportid;
- uint8_t nreports;
+ u_intnreports;
uint8_t *claimed;
};
-#define UHIDEV_CLAIM_MULTIPLE_REPORTID(u) \
- ((u)->reportid == __UHIDEV_CLAIM_MULTIPLE_REPORTID)
+#define UHIDEV_CLAIM_MULTIPLE_REPORTID(u) ((u)->claimed != NULL)
#define__UHIDEV_CLAIM_MULTIPLE_REPORTID255 /* XXX */
int uhidev_report_type_conv(int);
