Re: m_defrag(9) leak
On Thu, Sep 03, 2020 at 07:15:23AM +0200, Claudio Jeker wrote:
> On Thu, Sep 03, 2020 at 06:34:38AM +0200, Bjorn Ketelaars wrote:
> > The diff below should fix this. At least I'm able to build a kernel
> > (amd64 only), which doesn't explode when actually using it.
>
> This is probably not correct. m_defrag does not free the mbuf on failure
> because in the drivers the mbuf is most probably still sitting on the
> interace output queue and will be retried later on.
It might still be worth taking a careful look at each of these.
Even if you don't end up finding a bug, you'll end up understanding
driver code much better.
Generally if a driver leaks mbufs people will notice quickly.
But some drivers aren't used a lot and we've had some mbuf leaks being
noticed only after weeks. I fixed one mbuf leak in urtwn(4) in July,
where the bug was introduced in June (and was unrelated to m_defrag()).
In any case, such changes should be tested with relevant devices before commit.
As claudio says, when reviewing these you need to find out where the mbuf
came from and when it is supposed to be freed. In most cases drivers will
refer to mbufs by other data structures on lists or queues. These mbufs
must not be freed while they're still being referred to.
For example:
> > diff --git sys/arch/octeon/dev/if_cnmac.c sys/arch/octeon/dev/if_cnmac.c
> > index c45503c3d7f..c6a12d20a9a 100644
> > --- sys/arch/octeon/dev/if_cnmac.c
> > +++ sys/arch/octeon/dev/if_cnmac.c
> > @@ -755,8 +755,10 @@ cnmac_send_makecmd_gbuf(struct cnmac_softc *sc, struct
> > mbuf *m0,
> > return 0;
> >
> > defrag:
> > - if (m_defrag(m0, M_DONTWAIT) != 0)
> > + if (m_defrag(m0, M_DONTWAIT) != 0) {
> > + m_freem(m0);
> > return 1;
> > + }
> > gbuf[0] = cnmac_send_makecmd_w1(m0->m_len, KVTOPHYS(m0->m_data));
> > *rsegs = 1;
> > return 0;
In this instance, cnmac_start() will already free the mbuf on failure,
so your change introduces a double-free. On success, the mbuf ends up on
a list (via cnmac_send()) and will be freed later (via cnmac_free_task()).
Re: asn1/a_bitstring.c: zeroing after recallocarray
On Thu, Sep 03, 2020 at 07:03:14AM +0200, Theo Buehler wrote:
> The memset is not needed as recallocarray(3) does the zeroing already.
> (I also think the a->data == NULL check in the if clause is redundant,
> but I'm just suggesting to remove a bit that confused me)
ok,
-Otto
>
> Index: asn1/a_bitstr.c
> ===
> RCS file: /var/cvs/src/lib/libcrypto/asn1/a_bitstr.c,v
> retrieving revision 1.29
> diff -u -p -U7 -r1.29 a_bitstr.c
> --- asn1/a_bitstr.c 20 Oct 2018 16:07:09 - 1.29
> +++ asn1/a_bitstr.c 15 Jun 2020 12:46:00 -
> @@ -211,16 +211,14 @@ ASN1_BIT_STRING_set_bit(ASN1_BIT_STRING
> if ((a->length < (w + 1)) || (a->data == NULL)) {
> if (!value)
> return(1); /* Don't need to set */
> if ((c = recallocarray(a->data, a->length, w + 1, 1)) == NULL) {
> ASN1error(ERR_R_MALLOC_FAILURE);
> return 0;
> }
> - if (w + 1 - a->length > 0)
> - memset(c + a->length, 0, w + 1 - a->length);
> a->data = c;
> a->length = w + 1;
> }
> a->data[w] = ((a->data[w]) & iv) | v;
> while ((a->length > 0) && (a->data[a->length - 1] == 0))
> a->length--;
>
>
Re: m_defrag(9) leak
On Thu, Sep 03, 2020 at 06:34:38AM +0200, Bjorn Ketelaars wrote:
> On Tue 25/08/2020 08:42, Martin Pieuchot wrote:
> > Maxime Villard mentioned a leak due to a missing m_freem() in wg(4):
> > https://marc.info/?l=netbsd-tech-net&m=159827988018641&w=2
> >
> > It seems to be that such leak is present in other uses of m_defrag() in
> > the tree. I won't take the time to go though all of them, but an audit
> > would be welcome.
>
> Similar issue seems to exist in:
>
> sys/arch/octeon/dev/if_cnmac.c
> sys/arch/octeon/dev/if_ogx.c
> sys/dev/fdt/if_dwge.c
> sys/dev/fdt/if_dwxe.c
> sys/dev/ic/bcmgenet.c
> sys/dev/ic/gem.c
> sys/dev/ic/re.c
> sys/dev/ic/rt2860.c
> sys/dev/pci/if_bge.c
> sys/dev/pci/if_bnx.c
> sys/dev/pci/if_bnxt.c
> sys/dev/pci/if_bwfm_pci.c
> sys/dev/pci/if_cas.c
> sys/dev/pci/if_em.c
> sys/dev/pci/if_iavf.c
> sys/dev/pci/if_ix.c
> sys/dev/pci/if_ixl.c
> sys/dev/pci/if_mcx.c
> sys/dev/pci/if_msk.c
> sys/dev/pci/if_myx.c
> sys/dev/pci/if_rge.c
> sys/dev/pci/if_se.c
> sys/dev/pci/if_sis.c
> sys/dev/pci/if_sk.c
> sys/dev/pci/if_vge.c
> sys/dev/pci/if_vic.c
> sys/dev/pci/if_vmx.c
> sys/dev/pci/if_vr.c
> sys/dev/pv/if_hvn.c
> sys/dev/pv/if_vio.c
> sys/dev/pv/if_xnf.c
>
> The diff below should fix this. At least I'm able to build a kernel
> (amd64 only), which doesn't explode when actually using it.
This is probably not correct. m_defrag does not free the mbuf on failure
because in the drivers the mbuf is most probably still sitting on the
interace output queue and will be retried later on.
This is by design. wg(4) abused m_defrag for something else and its error
handling was just wrong.
> diff --git sys/arch/octeon/dev/if_cnmac.c sys/arch/octeon/dev/if_cnmac.c
> index c45503c3d7f..c6a12d20a9a 100644
> --- sys/arch/octeon/dev/if_cnmac.c
> +++ sys/arch/octeon/dev/if_cnmac.c
> @@ -755,8 +755,10 @@ cnmac_send_makecmd_gbuf(struct cnmac_softc *sc, struct
> mbuf *m0,
> return 0;
>
> defrag:
> - if (m_defrag(m0, M_DONTWAIT) != 0)
> + if (m_defrag(m0, M_DONTWAIT) != 0) {
> + m_freem(m0);
> return 1;
> + }
> gbuf[0] = cnmac_send_makecmd_w1(m0->m_len, KVTOPHYS(m0->m_data));
> *rsegs = 1;
> return 0;
> diff --git sys/arch/octeon/dev/if_ogx.c sys/arch/octeon/dev/if_ogx.c
> index 13a9cd32a7d..67695fabb23 100644
> --- sys/arch/octeon/dev/if_ogx.c
> +++ sys/arch/octeon/dev/if_ogx.c
> @@ -1243,8 +1243,10 @@ ogx_send_mbuf(struct ogx_softc *sc, struct mbuf *m0)
> }
>
> if (m != NULL) {
> - if (m_defrag(m0, M_DONTWAIT) != 0)
> + if (m_defrag(m0, M_DONTWAIT) != 0) {
> + m_freem(m0);
> return ENOMEM;
> + }
>
> /* Discard previously set fragments. */
> scroff -= sizeof(word) * nfrags;
> diff --git sys/dev/fdt/if_dwge.c sys/dev/fdt/if_dwge.c
> index 58114bf3e38..e9bef31fcdc 100644
> --- sys/dev/fdt/if_dwge.c
> +++ sys/dev/fdt/if_dwge.c
> @@ -1136,8 +1136,10 @@ dwge_encap(struct dwge_softc *sc, struct mbuf *m, int
> *idx)
> map = sc->sc_txbuf[cur].tb_map;
>
> if (bus_dmamap_load_mbuf(sc->sc_dmat, map, m, BUS_DMA_NOWAIT)) {
> - if (m_defrag(m, M_DONTWAIT))
> + if (m_defrag(m, M_DONTWAIT)) {
> + m_freem(m);
> return (EFBIG);
> + }
> if (bus_dmamap_load_mbuf(sc->sc_dmat, map, m, BUS_DMA_NOWAIT))
> return (EFBIG);
> }
> diff --git sys/dev/fdt/if_dwxe.c sys/dev/fdt/if_dwxe.c
> index e4895a9a044..a3c6122840a 100644
> --- sys/dev/fdt/if_dwxe.c
> +++ sys/dev/fdt/if_dwxe.c
> @@ -1198,8 +1198,10 @@ dwxe_encap(struct dwxe_softc *sc, struct mbuf *m, int
> *idx)
> map = sc->sc_txbuf[cur].tb_map;
>
> if (bus_dmamap_load_mbuf(sc->sc_dmat, map, m, BUS_DMA_NOWAIT)) {
> - if (m_defrag(m, M_DONTWAIT))
> + if (m_defrag(m, M_DONTWAIT)) {
> + m_freem(m);
> return (EFBIG);
> + }
> if (bus_dmamap_load_mbuf(sc->sc_dmat, map, m, BUS_DMA_NOWAIT))
> return (EFBIG);
> }
> diff --git sys/dev/ic/bcmgenet.c sys/dev/ic/bcmgenet.c
> index c9b21f7768b..2444b71a68a 100644
> --- sys/dev/ic/bcmgenet.c
> +++ sys/dev/ic/bcmgenet.c
> @@ -215,15 +215,19 @@ genet_setup_txbuf(struct genet_softc *sc, int index,
> struct mbuf *m)
>* than the minimum Ethernet packet size.
>*/
> if (m->m_len < ETHER_MIN_LEN - ETHER_CRC_LEN) {
> - if (m_defrag(m, M_DONTWAIT))
> + if (m_defrag(m, M_DONTWAIT)) {
> + m_freem(m);
> return 0;
> + }
> }
>
> error = bus_dmamap_load_mbuf(sc->sc_tx.buf_tag,
> sc->sc_tx.buf_map[index].map, m, BUS_DMA_WRITE | BUS_DMA_NOWAIT);
> if (error == EFBIG) {
> - if (m_defrag(m, M_DONTWAIT))
> + if (m_defrag(m, M_DONTWAIT)) {
> +
libcrypto: clean up asn1/x_info.c
There's this thing called calloc(3) and all variants of free(3) in
libcrypto are NULL safe.
PS: There's still many dozens of these to clean up. I don't intend to go
through these systematically anytime soon, but rather than letting this
one rot in my tree, I might as well ask for oks...
Index: asn1/x_info.c
===
RCS file: /var/cvs/src/lib/libcrypto/asn1/x_info.c,v
retrieving revision 1.17
diff -u -p -r1.17 x_info.c
--- asn1/x_info.c 29 Jan 2017 17:49:22 - 1.17
+++ asn1/x_info.c 14 Aug 2020 09:29:28 -
@@ -60,48 +60,35 @@
#include
#include
-#include
#include
X509_INFO *
X509_INFO_new(void)
{
- X509_INFO *ret = NULL;
+ X509_INFO *ret;
- ret = malloc(sizeof(X509_INFO));
- if (ret == NULL) {
+ if ((ret = calloc(1, sizeof(X509_INFO))) == NULL) {
ASN1error(ERR_R_MALLOC_FAILURE);
return (NULL);
}
-
- ret->enc_cipher.cipher = NULL;
- ret->enc_len = 0;
- ret->enc_data = NULL;
-
ret->references = 1;
- ret->x509 = NULL;
- ret->crl = NULL;
- ret->x_pkey = NULL;
- return (ret);
+
+ return ret;
}
void
X509_INFO_free(X509_INFO *x)
{
- int i;
-
if (x == NULL)
return;
- i = CRYPTO_add(&x->references, -1, CRYPTO_LOCK_X509_INFO);
- if (i > 0)
+ if (CRYPTO_add(&x->references, -1, CRYPTO_LOCK_X509_INFO) > 0)
return;
X509_free(x->x509);
- if (x->crl != NULL)
- X509_CRL_free(x->crl);
- if (x->x_pkey != NULL)
- X509_PKEY_free(x->x_pkey);
+ X509_CRL_free(x->crl);
+ X509_PKEY_free(x->x_pkey);
free(x->enc_data);
+
free(x);
}
asn1/a_bitstring.c: zeroing after recallocarray
The memset is not needed as recallocarray(3) does the zeroing already.
(I also think the a->data == NULL check in the if clause is redundant,
but I'm just suggesting to remove a bit that confused me)
Index: asn1/a_bitstr.c
===
RCS file: /var/cvs/src/lib/libcrypto/asn1/a_bitstr.c,v
retrieving revision 1.29
diff -u -p -U7 -r1.29 a_bitstr.c
--- asn1/a_bitstr.c 20 Oct 2018 16:07:09 - 1.29
+++ asn1/a_bitstr.c 15 Jun 2020 12:46:00 -
@@ -211,16 +211,14 @@ ASN1_BIT_STRING_set_bit(ASN1_BIT_STRING
if ((a->length < (w + 1)) || (a->data == NULL)) {
if (!value)
return(1); /* Don't need to set */
if ((c = recallocarray(a->data, a->length, w + 1, 1)) == NULL) {
ASN1error(ERR_R_MALLOC_FAILURE);
return 0;
}
- if (w + 1 - a->length > 0)
- memset(c + a->length, 0, w + 1 - a->length);
a->data = c;
a->length = w + 1;
}
a->data[w] = ((a->data[w]) & iv) | v;
while ((a->length > 0) && (a->data[a->length - 1] == 0))
a->length--;
Re: m_defrag(9) leak
On Tue 25/08/2020 08:42, Martin Pieuchot wrote:
> Maxime Villard mentioned a leak due to a missing m_freem() in wg(4):
> https://marc.info/?l=netbsd-tech-net&m=159827988018641&w=2
>
> It seems to be that such leak is present in other uses of m_defrag() in
> the tree. I won't take the time to go though all of them, but an audit
> would be welcome.
Similar issue seems to exist in:
sys/arch/octeon/dev/if_cnmac.c
sys/arch/octeon/dev/if_ogx.c
sys/dev/fdt/if_dwge.c
sys/dev/fdt/if_dwxe.c
sys/dev/ic/bcmgenet.c
sys/dev/ic/gem.c
sys/dev/ic/re.c
sys/dev/ic/rt2860.c
sys/dev/pci/if_bge.c
sys/dev/pci/if_bnx.c
sys/dev/pci/if_bnxt.c
sys/dev/pci/if_bwfm_pci.c
sys/dev/pci/if_cas.c
sys/dev/pci/if_em.c
sys/dev/pci/if_iavf.c
sys/dev/pci/if_ix.c
sys/dev/pci/if_ixl.c
sys/dev/pci/if_mcx.c
sys/dev/pci/if_msk.c
sys/dev/pci/if_myx.c
sys/dev/pci/if_rge.c
sys/dev/pci/if_se.c
sys/dev/pci/if_sis.c
sys/dev/pci/if_sk.c
sys/dev/pci/if_vge.c
sys/dev/pci/if_vic.c
sys/dev/pci/if_vmx.c
sys/dev/pci/if_vr.c
sys/dev/pv/if_hvn.c
sys/dev/pv/if_vio.c
sys/dev/pv/if_xnf.c
The diff below should fix this. At least I'm able to build a kernel
(amd64 only), which doesn't explode when actually using it.
diff --git sys/arch/octeon/dev/if_cnmac.c sys/arch/octeon/dev/if_cnmac.c
index c45503c3d7f..c6a12d20a9a 100644
--- sys/arch/octeon/dev/if_cnmac.c
+++ sys/arch/octeon/dev/if_cnmac.c
@@ -755,8 +755,10 @@ cnmac_send_makecmd_gbuf(struct cnmac_softc *sc, struct
mbuf *m0,
return 0;
defrag:
- if (m_defrag(m0, M_DONTWAIT) != 0)
+ if (m_defrag(m0, M_DONTWAIT) != 0) {
+ m_freem(m0);
return 1;
+ }
gbuf[0] = cnmac_send_makecmd_w1(m0->m_len, KVTOPHYS(m0->m_data));
*rsegs = 1;
return 0;
diff --git sys/arch/octeon/dev/if_ogx.c sys/arch/octeon/dev/if_ogx.c
index 13a9cd32a7d..67695fabb23 100644
--- sys/arch/octeon/dev/if_ogx.c
+++ sys/arch/octeon/dev/if_ogx.c
@@ -1243,8 +1243,10 @@ ogx_send_mbuf(struct ogx_softc *sc, struct mbuf *m0)
}
if (m != NULL) {
- if (m_defrag(m0, M_DONTWAIT) != 0)
+ if (m_defrag(m0, M_DONTWAIT) != 0) {
+ m_freem(m0);
return ENOMEM;
+ }
/* Discard previously set fragments. */
scroff -= sizeof(word) * nfrags;
diff --git sys/dev/fdt/if_dwge.c sys/dev/fdt/if_dwge.c
index 58114bf3e38..e9bef31fcdc 100644
--- sys/dev/fdt/if_dwge.c
+++ sys/dev/fdt/if_dwge.c
@@ -1136,8 +1136,10 @@ dwge_encap(struct dwge_softc *sc, struct mbuf *m, int
*idx)
map = sc->sc_txbuf[cur].tb_map;
if (bus_dmamap_load_mbuf(sc->sc_dmat, map, m, BUS_DMA_NOWAIT)) {
- if (m_defrag(m, M_DONTWAIT))
+ if (m_defrag(m, M_DONTWAIT)) {
+ m_freem(m);
return (EFBIG);
+ }
if (bus_dmamap_load_mbuf(sc->sc_dmat, map, m, BUS_DMA_NOWAIT))
return (EFBIG);
}
diff --git sys/dev/fdt/if_dwxe.c sys/dev/fdt/if_dwxe.c
index e4895a9a044..a3c6122840a 100644
--- sys/dev/fdt/if_dwxe.c
+++ sys/dev/fdt/if_dwxe.c
@@ -1198,8 +1198,10 @@ dwxe_encap(struct dwxe_softc *sc, struct mbuf *m, int
*idx)
map = sc->sc_txbuf[cur].tb_map;
if (bus_dmamap_load_mbuf(sc->sc_dmat, map, m, BUS_DMA_NOWAIT)) {
- if (m_defrag(m, M_DONTWAIT))
+ if (m_defrag(m, M_DONTWAIT)) {
+ m_freem(m);
return (EFBIG);
+ }
if (bus_dmamap_load_mbuf(sc->sc_dmat, map, m, BUS_DMA_NOWAIT))
return (EFBIG);
}
diff --git sys/dev/ic/bcmgenet.c sys/dev/ic/bcmgenet.c
index c9b21f7768b..2444b71a68a 100644
--- sys/dev/ic/bcmgenet.c
+++ sys/dev/ic/bcmgenet.c
@@ -215,15 +215,19 @@ genet_setup_txbuf(struct genet_softc *sc, int index,
struct mbuf *m)
* than the minimum Ethernet packet size.
*/
if (m->m_len < ETHER_MIN_LEN - ETHER_CRC_LEN) {
- if (m_defrag(m, M_DONTWAIT))
+ if (m_defrag(m, M_DONTWAIT)) {
+ m_freem(m);
return 0;
+ }
}
error = bus_dmamap_load_mbuf(sc->sc_tx.buf_tag,
sc->sc_tx.buf_map[index].map, m, BUS_DMA_WRITE | BUS_DMA_NOWAIT);
if (error == EFBIG) {
- if (m_defrag(m, M_DONTWAIT))
+ if (m_defrag(m, M_DONTWAIT)) {
+ m_freem(m);
return 0;
+ }
error = bus_dmamap_load_mbuf(sc->sc_tx.buf_tag,
sc->sc_tx.buf_map[index].map, m,
BUS_DMA_WRITE | BUS_DMA_NOWAIT);
diff --git sys/dev/ic/gem.c sys/dev/ic/gem.c
index 0cd66dbfd63..b5ef3eca8da 100644
--- sys/dev/ic/gem.c
+++ sys/dev/ic/gem.c
@@ -1698,6 +1698,7 @@ gem_load_mbuf(struct gem_softc *sc, struct gem_sxd *sd,
struct mbuf *m)
break;
/* FALLTHROUGH *
Re: httpd: use the original uri for REQUEST_URI
Let me update the diff. Previous doesn't have an error handling when
strdup() failed.
On Thu, 03 Sep 2020 13:02:51 +0900 (JST)
YASUOKA Masahiko wrote:
> The diff makes REQUEST_URI in FastCGI become the original request
> URI. Currently it is an url which is url decoded and canonicalized.
> I could not find a specification of REQUEST_URI, but I suppose it is
> the URI in HTTP request. Apache httpd and nginx is using the original
> URI for it.
>
> ok?
>
>
> Use the original requested URI for REQUEST_URI.
Index: usr.sbin/httpd/http.h
===
RCS file: /cvs/src/usr.sbin/httpd/http.h,v
retrieving revision 1.15
diff -u -p -r1.15 http.h
--- usr.sbin/httpd/http.h 8 May 2019 21:41:06 - 1.15
+++ usr.sbin/httpd/http.h 3 Sep 2020 04:09:26 -
@@ -246,6 +246,7 @@ struct http_descriptor {
/* Rewritten path and query remain NULL if not used */
char*http_path_alias;
char*http_query_alias;
+ char*http_path_orig;
/* A tree of headers and attached lists for repeated headers. */
struct kv *http_lastheader;
Index: usr.sbin/httpd/server_fcgi.c
===
RCS file: /cvs/src/usr.sbin/httpd/server_fcgi.c,v
retrieving revision 1.83
diff -u -p -r1.83 server_fcgi.c
--- usr.sbin/httpd/server_fcgi.c24 Aug 2020 15:49:11 - 1.83
+++ usr.sbin/httpd/server_fcgi.c3 Sep 2020 04:09:26 -
@@ -299,13 +299,13 @@ server_fcgi(struct httpd *env, struct cl
}
if (!desc->http_query) {
- if (fcgi_add_param(¶m, "REQUEST_URI", desc->http_path,
+ if (fcgi_add_param(¶m, "REQUEST_URI", desc->http_path_orig,
clt) == -1) {
errstr = "failed to encode param";
goto fail;
}
} else {
- if (asprintf(&str, "%s?%s", desc->http_path,
+ if (asprintf(&str, "%s?%s", desc->http_path_orig,
desc->http_query) == -1) {
errstr = "failed to encode param";
goto fail;
Index: usr.sbin/httpd/server_http.c
===
RCS file: /cvs/src/usr.sbin/httpd/server_http.c,v
retrieving revision 1.140
diff -u -p -r1.140 server_http.c
--- usr.sbin/httpd/server_http.c3 Aug 2020 10:59:53 - 1.140
+++ usr.sbin/httpd/server_http.c3 Sep 2020 04:09:26 -
@@ -100,6 +100,8 @@ server_httpdesc_free(struct http_descrip
free(desc->http_path);
desc->http_path = NULL;
+ free(desc->http_path_orig);
+ desc->http_path_orig = NULL;
free(desc->http_path_alias);
desc->http_path_alias = NULL;
free(desc->http_query);
@@ -1204,9 +1206,13 @@ server_response(struct httpd *httpd, str
char*hostval, *query;
const char *errstr = NULL;
- /* Decode the URL */
+ /* Preserve original path */
if (desc->http_path == NULL ||
- url_decode(desc->http_path) == NULL)
+ (desc->http_path_orig = strdup(desc->http_path)) == NULL)
+ goto fail;
+
+ /* Decode the URL */
+ if (url_decode(desc->http_path) == NULL)
goto fail;
/* Canonicalize the request path */
httpd: use the original uri for REQUEST_URI
The diff makes REQUEST_URI in FastCGI become the original request
URI. Currently it is an url which is url decoded and canonicalized.
I could not find a specification of REQUEST_URI, but I suppose it is
the URI in HTTP request. Apache httpd and nginx is using the original
URI for it.
ok?
Use the original requested URI for REQUEST_URI.
Index: usr.sbin/httpd/http.h
===
RCS file: /cvs/src/usr.sbin/httpd/http.h,v
retrieving revision 1.15
diff -u -p -r1.15 http.h
--- usr.sbin/httpd/http.h 8 May 2019 21:41:06 - 1.15
+++ usr.sbin/httpd/http.h 3 Sep 2020 04:00:49 -
@@ -246,6 +246,7 @@ struct http_descriptor {
/* Rewritten path and query remain NULL if not used */
char*http_path_alias;
char*http_query_alias;
+ char*http_path_orig;
/* A tree of headers and attached lists for repeated headers. */
struct kv *http_lastheader;
Index: usr.sbin/httpd/server_fcgi.c
===
RCS file: /cvs/src/usr.sbin/httpd/server_fcgi.c,v
retrieving revision 1.83
diff -u -p -r1.83 server_fcgi.c
--- usr.sbin/httpd/server_fcgi.c24 Aug 2020 15:49:11 - 1.83
+++ usr.sbin/httpd/server_fcgi.c3 Sep 2020 04:00:49 -
@@ -299,13 +299,13 @@ server_fcgi(struct httpd *env, struct cl
}
if (!desc->http_query) {
- if (fcgi_add_param(¶m, "REQUEST_URI", desc->http_path,
+ if (fcgi_add_param(¶m, "REQUEST_URI", desc->http_path_orig,
clt) == -1) {
errstr = "failed to encode param";
goto fail;
}
} else {
- if (asprintf(&str, "%s?%s", desc->http_path,
+ if (asprintf(&str, "%s?%s", desc->http_path_orig,
desc->http_query) == -1) {
errstr = "failed to encode param";
goto fail;
Index: usr.sbin/httpd/server_http.c
===
RCS file: /cvs/src/usr.sbin/httpd/server_http.c,v
retrieving revision 1.140
diff -u -p -r1.140 server_http.c
--- usr.sbin/httpd/server_http.c3 Aug 2020 10:59:53 - 1.140
+++ usr.sbin/httpd/server_http.c3 Sep 2020 04:00:49 -
@@ -100,6 +100,8 @@ server_httpdesc_free(struct http_descrip
free(desc->http_path);
desc->http_path = NULL;
+ free(desc->http_path_orig);
+ desc->http_path_orig = NULL;
free(desc->http_path_alias);
desc->http_path_alias = NULL;
free(desc->http_query);
@@ -1203,6 +1205,10 @@ server_response(struct httpd *httpd, str
int portval = -1, ret;
char*hostval, *query;
const char *errstr = NULL;
+
+ /* preserve original path */
+ if (desc->http_path != NULL)
+ desc->http_path_orig = strdup(desc->http_path);
/* Decode the URL */
if (desc->http_path == NULL ||
Re: [PATCH] Add common PCIE capability list
On Wed, Sep 02, 2020 at 03:19:55PM +1000, Jonathan Gray wrote:
> On Tue, Sep 01, 2020 at 11:44:03PM -0500, Jordan Hargrave wrote:
> > This patch adds a common function for scanning PCIE Express Capability list
> > The PCIE Capability list starts at 0x100 in extended PCI configuration
> > space.
>
> This seems to only handle extended capabilities?
> Something like pcie_get_ext_capability() would be a better name.
>
> It is 'PCI Express' not 'PCIExpress'
>
> 'ofs & 3' test doesn't make sense when PCI_PCIE_ECAP_NEXT() always
> masks those bits.
>
Ok fixed below...
> >
> > ---
> > sys/dev/pci/pci.c| 28
> > sys/dev/pci/pcivar.h | 2 ++
> > 2 files changed, 30 insertions(+)
> >
> > diff --git a/sys/dev/pci/pci.c b/sys/dev/pci/pci.c
> > index bf75f875e..8f9a5ef7a 100644
> > --- a/sys/dev/pci/pci.c
> > +++ b/sys/dev/pci/pci.c
> > @@ -677,6 +677,34 @@ pci_get_ht_capability(pci_chipset_tag_t pc, pcitag_t
> > tag, int capid,
> > return (0);
> > }
> >
> > +int
> > +pcie_get_capability(pci_chipset_tag_t pc, pcitag_t tag, int capid,
> > +int *offset, pcireg_t *value)
> > +{
> > + pcireg_t reg;
> > + unsigned int ofs;
> > +
> > + /* Make sure we support PCIExpress device */
> > + if (pci_get_capability(pc, tag, PCI_CAP_PCIEXPRESS, NULL, NULL) == 0)
> > + return (0);
> > + /* Scan PCIExpress capabilities */
> > + ofs = PCI_PCIE_ECAP;
> > + while (ofs != 0) {
> > + if ((ofs & 3) || (ofs < PCI_PCIE_ECAP))
> > + return (0);
> > + reg = pci_conf_read(pc, tag, ofs);
> > + if (PCI_PCIE_ECAP_ID(reg) == capid) {
> > + if (offset)
> > + *offset = ofs;
> > + if (value)
> > + *value = reg;
> > + return (1);
> > + }
> > + ofs = PCI_PCIE_ECAP_NEXT(reg);
> > + }
> > + return (0);
> > +}
> > +
> > uint16_t
> > pci_requester_id(pci_chipset_tag_t pc, pcitag_t tag)
> > {
> > diff --git a/sys/dev/pci/pcivar.h b/sys/dev/pci/pcivar.h
> > index bdfe0404f..0376ba992 100644
> > --- a/sys/dev/pci/pcivar.h
> > +++ b/sys/dev/pci/pcivar.h
> > @@ -233,6 +233,8 @@ int pci_io_find(pci_chipset_tag_t, pcitag_t, int,
> > bus_addr_t *,
> > intpci_mem_find(pci_chipset_tag_t, pcitag_t, int, bus_addr_t *,
> > bus_size_t *, int *);
> >
> > +intpcie_get_capability(pci_chipset_tag_t, pcitag_t, int,
> > + int *, pcireg_t *);
> > intpci_get_capability(pci_chipset_tag_t, pcitag_t, int,
> > int *, pcireg_t *);
> > intpci_get_ht_capability(pci_chipset_tag_t, pcitag_t, int,
> > --
> > 2.26.2
> >
> >
diff --git a/sys/dev/pci/pci.c b/sys/dev/pci/pci.c
index bf75f875e..56a85453a 100644
--- a/sys/dev/pci/pci.c
+++ b/sys/dev/pci/pci.c
@@ -677,6 +677,34 @@ pci_get_ht_capability(pci_chipset_tag_t pc, pcitag_t tag,
int capid,
return (0);
}
+int
+pci_get_ext_capability(pci_chipset_tag_t pc, pcitag_t tag, int capid,
+int *offset, pcireg_t *value)
+{
+ pcireg_t reg;
+ unsigned int ofs;
+
+ /* Make sure we support PCI Express device */
+ if (pci_get_capability(pc, tag, PCI_CAP_PCIEXPRESS, NULL, NULL) == 0)
+ return (0);
+ /* Scan PCI Express capabilities */
+ ofs = PCI_PCIE_ECAP;
+ while (ofs != 0) {
+ if (ofs < PCI_PCIE_ECAP)
+ return (0);
+ reg = pci_conf_read(pc, tag, ofs);
+ if (PCI_PCIE_ECAP_ID(reg) == capid) {
+ if (offset)
+ *offset = ofs;
+ if (value)
+ *value = reg;
+ return (1);
+ }
+ ofs = PCI_PCIE_ECAP_NEXT(reg);
+ }
+ return (0);
+}
+
uint16_t
pci_requester_id(pci_chipset_tag_t pc, pcitag_t tag)
{
diff --git a/sys/dev/pci/pcivar.h b/sys/dev/pci/pcivar.h
index bdfe0404f..1a19996f5 100644
--- a/sys/dev/pci/pcivar.h
+++ b/sys/dev/pci/pcivar.h
@@ -237,6 +237,8 @@ int pci_get_capability(pci_chipset_tag_t, pcitag_t, int,
int *, pcireg_t *);
intpci_get_ht_capability(pci_chipset_tag_t, pcitag_t, int,
int *, pcireg_t *);
+intpci_get_ext_capability(pci_chipset_tag_t, pcitag_t, int,
+ int *, pcireg_t *);
struct msix_vector;
clang warning in yacc's output.c
This one is a bit less immediate than the vmctl one:
/usr/src/usr.bin/yacc/output.c:908:26: warning: comparing a pointer to a null
character
constant; did you mean to compare to NULL? [-Wpointer-compare]
if ((s = symnam[i]) != '\0') {
^~~~
(void *)0
This time the suggestion is correct. It used to be 'if (s = symnam[i])'
and the incorrect spelling of NULL was chosen in a -Wall cleanup 19 years
ago (-r 1.6). Upstream uses a naked 0 instead of NULL, so does NetBSD.
Index: output.c
===
RCS file: /var/cvs/src/usr.bin/yacc/output.c,v
retrieving revision 1.27
diff -u -p -r1.27 output.c
--- output.c23 May 2020 21:08:38 - 1.27
+++ output.c2 Sep 2020 20:44:13 -
@@ -905,7 +905,7 @@ output_debug(void)
"\t{", symbol_prefix);
j = 80;
for (i = 0; i <= max; ++i) {
- if ((s = symnam[i]) != '\0') {
+ if ((s = symnam[i]) != NULL) {
if (s[0] == '"') {
k = 7;
while (*++s != '"') {
Re: clang warning in vmctl
Looks correct, OK millert@ - todd On Wed, 02 Sep 2020 21:43:47 +0200, Theo Buehler wrote: > clang 10 complains: > > /usr/src/usr.sbin/vmctl/vmctl.c:792:35: warning: comparing a pointer to a nul > l character > constant; did you mean to compare to NULL? [-Wpointer-compare] > '/')) == NULL || ++tty == '\0') > ^~~~ > (void *)0 > The intent is probably rather this: > > Index: vmctl.c > === > RCS file: /var/cvs/src/usr.sbin/vmctl/vmctl.c,v > retrieving revision 1.74 > diff -u -p -r1.74 vmctl.c > --- vmctl.c 11 Mar 2020 12:47:49 - 1.74 > +++ vmctl.c 2 Sep 2020 18:06:47 - > @@ -789,7 +789,7 @@ print_vm_info(struct vmop_info_result *l > tty = "-"; > /* get tty - skip /dev/ path */ > else if ((tty = strrchr(vmi->vir_ttyname, > - '/')) == NULL || ++tty == '\0') > + '/')) == NULL || *++tty == '\0') > tty = list[i].vir_ttyname; > > (void)fmt_scaled(vir->vir_used_size, curmem); >
Re: clang warning in vmctl
I had this diff in my tree for a while, but haven't gotten around
to passing it up.
there are other warnings in the tree, found by newer clang, that
people should review. Some of them are just noise, in particular
relating to missing {}.
Theo Buehler wrote:
> clang 10 complains:
>
> /usr/src/usr.sbin/vmctl/vmctl.c:792:35: warning: comparing a pointer to a
> null character
> constant; did you mean to compare to NULL? [-Wpointer-compare]
> '/')) == NULL || ++tty == '\0')
> ^~~~
> (void *)0
> The intent is probably rather this:
>
> Index: vmctl.c
> ===
> RCS file: /var/cvs/src/usr.sbin/vmctl/vmctl.c,v
> retrieving revision 1.74
> diff -u -p -r1.74 vmctl.c
> --- vmctl.c 11 Mar 2020 12:47:49 - 1.74
> +++ vmctl.c 2 Sep 2020 18:06:47 -
> @@ -789,7 +789,7 @@ print_vm_info(struct vmop_info_result *l
> tty = "-";
> /* get tty - skip /dev/ path */
> else if ((tty = strrchr(vmi->vir_ttyname,
> - '/')) == NULL || ++tty == '\0')
> + '/')) == NULL || *++tty == '\0')
> tty = list[i].vir_ttyname;
>
> (void)fmt_scaled(vir->vir_used_size, curmem);
>
clang warning in vmctl
clang 10 complains: /usr/src/usr.sbin/vmctl/vmctl.c:792:35: warning: comparing a pointer to a null character constant; did you mean to compare to NULL? [-Wpointer-compare] '/')) == NULL || ++tty == '\0') ^~~~ (void *)0 The intent is probably rather this: Index: vmctl.c === RCS file: /var/cvs/src/usr.sbin/vmctl/vmctl.c,v retrieving revision 1.74 diff -u -p -r1.74 vmctl.c --- vmctl.c 11 Mar 2020 12:47:49 - 1.74 +++ vmctl.c 2 Sep 2020 18:06:47 - @@ -789,7 +789,7 @@ print_vm_info(struct vmop_info_result *l tty = "-"; /* get tty - skip /dev/ path */ else if ((tty = strrchr(vmi->vir_ttyname, - '/')) == NULL || ++tty == '\0') + '/')) == NULL || *++tty == '\0') tty = list[i].vir_ttyname; (void)fmt_scaled(vir->vir_used_size, curmem);
pms(4): fix "pointer skipping" for certain v1 elantech touchpads
So, continuing with fixing my netbook, I have another patch which fixes
"pointer skipping". This is a bug in 2 particular (0x20022 and 0x20600)
elantech
v1 firmwares which causes at least the first packet (in my tests, only
one
packet was mangled, but Linux says two and I don't have a way to test it
with
0x20600 FW, so I'm just dropping two packets, since it doesn't change
anything
for my FW) to state the last position and not the current one, thus
jumping the
cursor somewhere else.
I'm not really sure about the counter variable, can it be like this or
should it
get moved to the softc struct or even somewhere else?
Index: pms.c
===
RCS file: /cvs/src/sys/dev/pckbc/pms.c,v
retrieving revision 1.94
diff -u -p -u -p -r1.94 pms.c
--- pms.c 10 Aug 2020 21:55:59 - 1.94
+++ pms.c 2 Sep 2020 14:54:48 -
@@ -2425,8 +2425,26 @@ void
pms_proc_elantech_v1(struct pms_softc *sc)
{
struct elantech_softc *elantech = sc->elantech;
+ static int single_touch_pkt = 0;
int x, y, w, z;
u_int buttons;
+
+ /*
+* Firmwares 0x20022 and 0x20600 have a bug, which makes the first 2
+* position reports mangled, which in turns causes cursor skipping.
+*/
+
+ if (elantech->fw_version == 0x20022 || elantech->fw_version ==
0x20600) {
+ if((sc->packet[0] & 0xc0) >> 6)
+ {
+ if(single_touch_pkt < 2)
+ {
+ single_touch_pkt++;
+ return;
+ }
+ } else
+ single_touch_pkt = 0;
+ }
buttons = butmap[sc->packet[0] & 3];
Re: ldom.conf.5: clarify vcpu strides
On Wed, Sep 02, 2020 at 04:41:49PM +0200, Klemens Nanni wrote:
> They way strides work is everything but intuitive and the manual doesn't
> really help; I've had multiple hackers/users ask me how to use them.
>
> `vcpu 8' assigns eight virtual CPUs to a domain.
>
> `vcpu 8:2' allocates eight VCPUs two times but assigns eight VCPUs
> only once, leaving the other eight allocated (read: unusable) but not
> assigned to any domain.
>
> `vcpu 8:3' would allocate 24 VCPUs and assign eight to a domain.
>
> This multiplicative property is not obvious from the manual; the way I
> read the current wording is `vcpu 8:2' allocating ten VCPUs and assign
> eight, i.e. the stride being an additive count.
>
> stsp brought this up and we came up with the following diff.
> Feedback? OK?
My problem with the existing wording is that it says a "stride" exists
but semantics are not specified. I ended up assuming stride corresponded
to the amount of unused CPUs (e.g. 8:4 would mean allocate 8 CPUs, and
leave 4 of those CPUs unused).
I could only find out how it actually works by reading code.
I would like to suggest an example for the EXAMPLES section which
illustrates how a suitable stride factor can be determined (divide the
number of desired "unused" cpus by the number of desired "used" cpus):
diff f2b93fb3c1fc6097f79ea2bfd28de3a4ab8b938b /usr/src
blob - 1d72ee44a4ab333146b004907e240dfd939f11c5
file + usr.sbin/ldomctl/ldom.conf.5
--- usr.sbin/ldomctl/ldom.conf.5
+++ usr.sbin/ldomctl/ldom.conf.5
@@ -112,6 +112,12 @@ domain "salmah" {
.Pp
On a machine with 32 cores and 64GB physical memory, this leaves 12 cores and
58GB memory to the primary domain.
+.Pp
+In order to use 2 CPUs out of 8, leaving 6 CPUs allocated but unused,
+a stride factor of 3 must be used (6 divided by 2):
+.Bd -literal -offset indent
+ vcpu 2:3
+.Ed
.Sh SEE ALSO
.Xr eeprom 8 ,
.Xr ldomctl 8 ,
Re: wg: count peers per interface not globally
On Wed, Sep 02, 2020 at 04:12:33PM +1000, Matt Dunwoodie wrote: > The patch isn't fully correct. When you remove a peer, sc_peer_num > will decrement, and if you add a new peer afterwards you will have > duplicate IDs. This is likely to create further headaches. A dedicated > ID counter in wg_softc should probably be used. Yes, it's just a small improvement and removing individual peers brings counting out of sync, although I'd argue its still more useful than the global counter. I can give proper peer counting a try soon.
ldom.conf.5: clarify vcpu strides
They way strides work is everything but intuitive and the manual doesn't really help; I've had multiple hackers/users ask me how to use them. `vcpu 8' assigns eight virtual CPUs to a domain. `vcpu 8:2' allocates eight VCPUs two times but assigns eight VCPUs only once, leaving the other eight allocated (read: unusable) but not assigned to any domain. `vcpu 8:3' would allocate 24 VCPUs and assign eight to a domain. This multiplicative property is not obvious from the manual; the way I read the current wording is `vcpu 8:2' allocating ten VCPUs and assign eight, i.e. the stride being an additive count. stsp brought this up and we came up with the following diff. Feedback? OK? Index: ldom.conf.5 === RCS file: /cvs/src/usr.sbin/ldomctl/ldom.conf.5,v retrieving revision 1.13 diff -u -p -r1.13 ldom.conf.5 --- ldom.conf.5 21 Feb 2020 19:39:28 - 1.13 +++ ldom.conf.5 2 Sep 2020 14:26:58 - @@ -38,8 +38,13 @@ If no configuration for the primary doma all CPU and memory resources not used by any guest domains. .It Ic vcpu Ar number Ns Op : Ns Ar stride Declare the number of virtual CPUs assigned to a domain. -Optionally a stride can be specified to allocate additional virtual CPUs -but not assign them to a domain. +Optionally a stride factor can be specified to allocate +.Ar number +virtual CPUs +.Ar stride +times but not assign more than +.Ar number +virtual CPUs to a domain, leaving the rest unassigned. This can be used to distribute virtual CPUs over the available CPU cores. .It Ic memory Ar bytes Declare the amount of memory assigned to a domain, in bytes.
Re: shrinking and growing reallocs: a theoretical? bad case for performance
On Tue, Sep 01, 2020 at 11:56:36AM +0100, Stuart Henderson wrote: > On 2020/08/31 08:39, Otto Moerbeek wrote: > > A question from Theo made me think about realloc and come up with a > > particular bad case for performance. I do not know if it happens in > > practice, but it was easy to create a test program to hit the case. > > Not very scientific testing (a single attempt at building one port), but > this seems to help quite a lot when compiling programs written in rust. > I encourage others to test the diff :-) > It turned out this particular case was a fluke. But I'm still very interested in cases where it does matter and tests in general as well. -Otto
Re: ospf6d: use ROUTE_FLAGFILTER
On Wed, Sep 02, 2020 at 03:23:28PM +1000, Jonathan Matthew wrote:
> Like ospfd, ospf6d can use ROUTE_FLAGFILTER to opt out of receiving messages
> relating to L2 and broadcast routes on its routing socket. We've been running
> this for a week or so with no problems.
>
> ok?
ok remi@
>
> Index: kroute.c
> ===
> RCS file: /cvs/src/usr.sbin/ospf6d/kroute.c,v
> retrieving revision 1.64
> diff -u -p -u -p -r1.64 kroute.c
> --- kroute.c 17 May 2020 18:29:25 - 1.64
> +++ kroute.c 18 Aug 2020 11:56:09 -
> @@ -102,6 +102,7 @@ kr_init(int fs, u_int rdomain, int redis
> int opt = 0, rcvbuf, default_rcvbuf;
> socklen_t optlen;
> int filter_prio = fib_prio;
> + int filter_flags = RTF_LLINFO | RTF_BROADCAST;
>
> kr_state.fib_sync = fs;
> kr_state.rdomain = rdomain;
> @@ -127,6 +128,12 @@ kr_init(int fs, u_int rdomain, int redis
> if (setsockopt(kr_state.fd, AF_ROUTE, ROUTE_PRIOFILTER, &filter_prio,
> sizeof(filter_prio)) == -1) {
> log_warn("%s: setsockopt AF_ROUTE ROUTE_PRIOFILTER", __func__);
> + /* not fatal */
> + }
> +
> + if (setsockopt(kr_state.fd, AF_ROUTE, ROUTE_FLAGFILTER, &filter_flags,
> + sizeof(filter_flags)) == -1) {
> + log_warn("%s: setsockopt AF_ROUTE ROUTE_FLAGFILTER", __func__);
> /* not fatal */
> }
>
>
