CVS commit: src/sys/dev/pci
Module Name:src Committed By: msaitoh Date: Fri Nov 8 04:45:55 UTC 2019 Modified Files: src/sys/dev/pci: pcidevs.h pcidevs_data.h Log Message: Regen. To generate a diff of this commit: cvs rdiff -u -r1.1377 -r1.1378 src/sys/dev/pci/pcidevs.h cvs rdiff -u -r1.1376 -r1.1377 src/sys/dev/pci/pcidevs_data.h Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files.
CVS commit: src/sys/dev/pci
Module Name:src Committed By: msaitoh Date: Fri Nov 8 04:37:45 UTC 2019 Modified Files: src/sys/dev/pci: pcidevs Log Message: - Update Intel's NVMe SSDs. - Modify 0x0953's description to "750 or DC P3[567]00 SSD" - Add DC P4[56]00 - Add Apollo Lake TXE HECI. To generate a diff of this commit: cvs rdiff -u -r1.1389 -r1.1390 src/sys/dev/pci/pcidevs Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files. Modified files: Index: src/sys/dev/pci/pcidevs diff -u src/sys/dev/pci/pcidevs:1.1389 src/sys/dev/pci/pcidevs:1.1390 --- src/sys/dev/pci/pcidevs:1.1389 Tue Oct 29 16:31:48 2019 +++ src/sys/dev/pci/pcidevs Fri Nov 8 04:37:45 2019 @@ -1,4 +1,4 @@ -$NetBSD: pcidevs,v 1.1389 2019/10/29 16:31:48 msaitoh Exp $ +$NetBSD: pcidevs,v 1.1390 2019/11/08 04:37:45 msaitoh Exp $ /* * Copyright (c) 1995, 1996 Christopher G. Demetriou @@ -3117,7 +3117,7 @@ product INTEL X1000_HS_UART 0x0936 Quark product INTEL X1000_MAC 0x0937 Quark X1000 10/100 Ethernet MAC product INTEL X1000_EHCI 0x0939 Quark X1000 EHCI product INTEL X1000_OHCI 0x093a Quark X1000 OHCI -product INTEL PCIE_NVME_SSD 0x0953 PCIe NVMe SSD +product INTEL PCIE_NVME_SSD 0x0953 750 or DC P3[567]00 SSD product INTEL X1000_HB 0x0958 Quark X1000 Host Bridge product INTEL WIFI_LINK_7265_1 0x095a Dual Band Wireless AC 7265 product INTEL WIFI_LINK_7265_2 0x095b Dual Band Wireless AC 7265 @@ -3142,6 +3142,8 @@ product INTEL CORE4G_S_ULT_GT3 0x0a2a HD product INTEL CORE4G_R_ULT_GT3_1 0x0a2b HD Graphics product INTEL CORE4G_R_ULT_GT3_2 0x0a2e Iris Graphics 5100 product INTEL DC_P3520_SSD 0x0a53 SSD DC P3520 +product INTEL DC_P4500_SSD 0x0a54 SSD DC P4500 +product INTEL DC_P4600_SSD 0x0a55 SSD DC P4600 product INTEL HASWELL_HOST_DRAM 0x0c00 Haswell Host Bridge, DRAM product INTEL HASWELL_PCIE16 0x0c01 Haswell PCI-E x16 Controller product INTEL HASWELL_PCIE8 0x0c05 Haswell PCI-E x8 Controller @@ -4944,6 +4946,9 @@ product INTEL APL_P2SB 0x5a92 Apollo La product INTEL APL_PMC 0x5a94 Apollo Lake PMC product INTEL APL_FASTSPI 0x5a96 Apollo Lake Fast SPI product INTEL APL_HDA 0x5a98 Apollo Lake HD Audio +product INTEL APL_TXE_HECI_1 0x5a9a Apollo Lake TXE HECI1 +product INTEL APL_TXE_HECI_2 0x5a9c Apollo Lake TXE HECI2 +product INTEL APL_TXE_HECI_3 0x5a9e Apollo Lake TXE HECI3 product INTEL APL_ISH 0x5aa2 Apollo Lake Integrated Sensor Hub product INTEL APL_XHCI 0x5aa8 Apollo Lake USB Host (xHCI) product INTEL APL_XDCI 0x5aaa Apollo Lake USB Device (xDCI)
CVS commit: src/sys/dev/pci
Module Name:src Committed By: msaitoh Date: Fri Nov 8 04:37:45 UTC 2019 Modified Files: src/sys/dev/pci: pcidevs Log Message: - Update Intel's NVMe SSDs. - Modify 0x0953's description to "750 or DC P3[567]00 SSD" - Add DC P4[56]00 - Add Apollo Lake TXE HECI. To generate a diff of this commit: cvs rdiff -u -r1.1389 -r1.1390 src/sys/dev/pci/pcidevs Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files.
CVS commit: src/sys/arch/x86/x86
Module Name:src Committed By: msaitoh Date: Fri Nov 8 04:15:02 UTC 2019 Modified Files: src/sys/arch/x86/x86: intr.c Log Message: Fix a bug that evcnt_detach() called twice when the idt vector is full. OK'd by knakahara. To generate a diff of this commit: cvs rdiff -u -r1.146 -r1.147 src/sys/arch/x86/x86/intr.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files.
CVS commit: src/sys/arch/x86/x86
Module Name:src
Committed By: msaitoh
Date: Fri Nov 8 04:15:02 UTC 2019
Modified Files:
src/sys/arch/x86/x86: intr.c
Log Message:
Fix a bug that evcnt_detach() called twice when the idt vector is full.
OK'd by knakahara.
To generate a diff of this commit:
cvs rdiff -u -r1.146 -r1.147 src/sys/arch/x86/x86/intr.c
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
Modified files:
Index: src/sys/arch/x86/x86/intr.c
diff -u src/sys/arch/x86/x86/intr.c:1.146 src/sys/arch/x86/x86/intr.c:1.147
--- src/sys/arch/x86/x86/intr.c:1.146 Mon Jun 17 06:38:30 2019
+++ src/sys/arch/x86/x86/intr.c Fri Nov 8 04:15:02 2019
@@ -1,4 +1,4 @@
-/* $NetBSD: intr.c,v 1.146 2019/06/17 06:38:30 msaitoh Exp $ */
+/* $NetBSD: intr.c,v 1.147 2019/11/08 04:15:02 msaitoh Exp $ */
/*
* Copyright (c) 2007, 2008, 2009 The NetBSD Foundation, Inc.
@@ -133,7 +133,7 @@
*/
#include
-__KERNEL_RCSID(0, "$NetBSD: intr.c,v 1.146 2019/06/17 06:38:30 msaitoh Exp $");
+__KERNEL_RCSID(0, "$NetBSD: intr.c,v 1.147 2019/11/08 04:15:02 msaitoh Exp $");
#include "opt_intrdebug.h"
#include "opt_multiprocessor.h"
@@ -475,8 +475,10 @@ intr_free_io_intrsource_direct(struct in
SIMPLEQ_REMOVE(_interrupt_sources, isp, intrsource, is_list);
/* Is this interrupt established? */
- if (isp->is_evname[0] != '\0')
+ if (isp->is_evname[0] != '\0') {
evcnt_detach(>is_evcnt);
+ isp->is_evname[0] = '\0';
+ }
kmem_free(isp->is_saved_evcnt,
sizeof(*(isp->is_saved_evcnt)) * ncpu);
@@ -679,6 +681,7 @@ intr_allocate_slot(struct pic *pic, int
}
if (idtvec == 0) {
evcnt_detach(>ci_isources[slot]->is_evcnt);
+ ci->ci_isources[slot]->is_evname[0] = '\0';
ci->ci_isources[slot] = NULL;
return EBUSY;
}
CVS commit: src/sys/arch/arm/rockchip
Module Name:src Committed By: jmcneill Date: Fri Nov 8 00:35:16 UTC 2019 Modified Files: src/sys/arch/arm/rockchip: rk_i2c.c Log Message: Support reads of more than 32 bytes in a single xfer. To generate a diff of this commit: cvs rdiff -u -r1.5 -r1.6 src/sys/arch/arm/rockchip/rk_i2c.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files.
CVS commit: src/sys/arch/arm/rockchip
Module Name:src
Committed By: jmcneill
Date: Fri Nov 8 00:35:16 UTC 2019
Modified Files:
src/sys/arch/arm/rockchip: rk_i2c.c
Log Message:
Support reads of more than 32 bytes in a single xfer.
To generate a diff of this commit:
cvs rdiff -u -r1.5 -r1.6 src/sys/arch/arm/rockchip/rk_i2c.c
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
Modified files:
Index: src/sys/arch/arm/rockchip/rk_i2c.c
diff -u src/sys/arch/arm/rockchip/rk_i2c.c:1.5 src/sys/arch/arm/rockchip/rk_i2c.c:1.6
--- src/sys/arch/arm/rockchip/rk_i2c.c:1.5 Wed Sep 18 12:49:34 2019
+++ src/sys/arch/arm/rockchip/rk_i2c.c Fri Nov 8 00:35:16 2019
@@ -1,4 +1,4 @@
-/* $NetBSD: rk_i2c.c,v 1.5 2019/09/18 12:49:34 tnn Exp $ */
+/* $NetBSD: rk_i2c.c,v 1.6 2019/11/08 00:35:16 jmcneill Exp $ */
/*-
* Copyright (c) 2018 Jared McNeill
@@ -28,7 +28,7 @@
#include
-__KERNEL_RCSID(0, "$NetBSD: rk_i2c.c,v 1.5 2019/09/18 12:49:34 tnn Exp $");
+__KERNEL_RCSID(0, "$NetBSD: rk_i2c.c,v 1.6 2019/11/08 00:35:16 jmcneill Exp $");
#include
#include
@@ -284,7 +284,7 @@ rk_i2c_write(struct rk_i2c_softc *sc, i2
static int
rk_i2c_read(struct rk_i2c_softc *sc, i2c_addr_t addr,
const uint8_t *cmd, size_t cmdlen, uint8_t *buf,
-size_t buflen, int flags, bool send_start)
+size_t buflen, int flags, bool send_start, bool last_ack)
{
uint32_t rxdata[8];
uint32_t con, mrxaddr, mrxraddr;
@@ -296,24 +296,27 @@ rk_i2c_read(struct rk_i2c_softc *sc, i2c
if (cmdlen > 3)
return EINVAL;
- mode = RKI2C_CON_I2C_MODE_RTX;
- con = RKI2C_CON_I2C_EN | RKI2C_CON_ACK | __SHIFTIN(mode, RKI2C_CON_I2C_MODE);
+ mode = send_start ? RKI2C_CON_I2C_MODE_RTX : RKI2C_CON_I2C_MODE_RX;
+ con = RKI2C_CON_I2C_EN | __SHIFTIN(mode, RKI2C_CON_I2C_MODE);
WR4(sc, RKI2C_CON, con);
if (send_start && (error = rk_i2c_start(sc)) != 0)
return error;
- mrxaddr = __SHIFTIN((addr << 1) | 1, RKI2C_MRXADDR_SADDR) |
- RKI2C_MRXADDR_ADDLVLD;
- WR4(sc, RKI2C_MRXADDR, mrxaddr);
- for (n = 0, mrxraddr = 0; n < cmdlen; n++) {
- mrxraddr |= cmd[n] << (n * 8);
- mrxraddr |= (RKI2C_MRXRADDR_ADDLVLD << n);
+ if (send_start) {
+ mrxaddr = __SHIFTIN((addr << 1) | 1, RKI2C_MRXADDR_SADDR) |
+ RKI2C_MRXADDR_ADDLVLD;
+ WR4(sc, RKI2C_MRXADDR, mrxaddr);
+ for (n = 0, mrxraddr = 0; n < cmdlen; n++) {
+ mrxraddr |= cmd[n] << (n * 8);
+ mrxraddr |= (RKI2C_MRXRADDR_ADDLVLD << n);
+ }
+ WR4(sc, RKI2C_MRXRADDR, mrxraddr);
}
- WR4(sc, RKI2C_MRXRADDR, mrxraddr);
- /* Acknowledge last byte read */
- con |= RKI2C_CON_ACK;
+ if (last_ack) {
+ con |= RKI2C_CON_ACK;
+ }
WR4(sc, RKI2C_CON, con);
/* Receive data. Slave address goes in the lower 8 bits of MRXADDR */
@@ -321,8 +324,14 @@ rk_i2c_read(struct rk_i2c_softc *sc, i2c
if ((error = rk_i2c_wait(sc, RKI2C_IPD_MBRFIPD)) != 0)
return error;
+#if 0
bus_space_read_region_4(sc->sc_bst, sc->sc_bsh, RKI2C_RXDATA(0),
rxdata, howmany(buflen, 4));
+#else
+ for (n = 0; n < roundup(buflen, 4); n += 4)
+ rxdata[n/4] = RD4(sc, RKI2C_RXDATA(n/4));
+#endif
+
memcpy(buf, rxdata, buflen);
return 0;
@@ -339,7 +348,19 @@ rk_i2c_exec(void *priv, i2c_op_t op, i2c
KASSERT(mutex_owned(>sc_lock));
if (I2C_OP_READ_P(op)) {
- error = rk_i2c_read(sc, addr, cmdbuf, cmdlen, buf, buflen, flags, send_start);
+ uint8_t *databuf = buf;
+ while (buflen > 0) {
+ const size_t datalen = uimin(buflen, 32);
+ const bool last_ack = datalen == buflen;
+ error = rk_i2c_read(sc, addr, cmdbuf, cmdlen, databuf, datalen, flags, send_start, last_ack);
+ if (error != 0)
+break;
+ databuf += datalen;
+ buflen -= datalen;
+ send_start = false;
+ cmdbuf = NULL;
+ cmdlen = 0;
+ }
} else {
error = rk_i2c_write(sc, addr, cmdbuf, cmdlen, buf, buflen, flags, send_start);
}
CVS commit: src/lib/libc/tls
Module Name:src
Committed By: joerg
Date: Thu Nov 7 22:25:22 UTC 2019
Modified Files:
src/lib/libc/tls: tls.c
Log Message:
Mirror the ld.elf_so logic for handling aligning the TLS size.
Most noticable, recompute the start of the TLS area for variant I
relative to the TCB. This makes a difference when the segment size and
base alignment don't agree.
To generate a diff of this commit:
cvs rdiff -u -r1.11 -r1.12 src/lib/libc/tls/tls.c
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
Modified files:
Index: src/lib/libc/tls/tls.c
diff -u src/lib/libc/tls/tls.c:1.11 src/lib/libc/tls/tls.c:1.12
--- src/lib/libc/tls/tls.c:1.11 Tue Nov 5 22:22:42 2019
+++ src/lib/libc/tls/tls.c Thu Nov 7 22:25:21 2019
@@ -1,4 +1,4 @@
-/* $NetBSD: tls.c,v 1.11 2019/11/05 22:22:42 joerg Exp $ */
+/* $NetBSD: tls.c,v 1.12 2019/11/07 22:25:21 joerg Exp $ */
/*-
* Copyright (c) 2011 The NetBSD Foundation, Inc.
@@ -30,7 +30,7 @@
*/
#include
-__RCSID("$NetBSD: tls.c,v 1.11 2019/11/05 22:22:42 joerg Exp $");
+__RCSID("$NetBSD: tls.c,v 1.12 2019/11/07 22:25:21 joerg Exp $");
#include "namespace.h"
@@ -86,14 +86,16 @@ _rtld_tls_allocate(void)
if (initial_thread_tcb == NULL) {
#ifdef __HAVE_TLS_VARIANT_II
- tls_size = roundup2(tls_size, alignof(max_align_t));
+ tls_allocation = roundup2(tls_size, alignof(max_align_t));
+#else
+ tls_allocation = tls_size;
#endif
- tls_allocation = tls_size + sizeof(*tcb);
- initial_thread_tcb = p = mmap(NULL, tls_allocation,
- PROT_READ | PROT_WRITE, MAP_ANON, -1, 0);
+ initial_thread_tcb = p = mmap(NULL,
+ tls_allocation + sizeof(*tcb), PROT_READ | PROT_WRITE,
+ MAP_ANON, -1, 0);
} else {
- p = calloc(1, tls_allocation);
+ p = calloc(1, tls_allocation + sizeof(*tcb));
}
if (p == NULL) {
static const char msg[] = "TLS allocation failed, terminating\n";
@@ -106,7 +108,8 @@ _rtld_tls_allocate(void)
p += sizeof(struct tls_tcb);
#else
/* LINTED tls_size is rounded above */
- tcb = (struct tls_tcb *)(p + tls_size);
+ tcb = (struct tls_tcb *)(p + tls_allocation);
+ p = (uint8_t *)tcb - tls_size;
tcb->tcb_self = tcb;
#endif
memcpy(p, tls_initaddr, tls_initsize);
@@ -126,10 +129,10 @@ _rtld_tls_free(struct tls_tcb *tcb)
p = (uint8_t *)tcb;
#else
/* LINTED */
- p = (uint8_t *)tcb - tls_size;
+ p = (uint8_t *)tcb - tls_allocation;
#endif
if (p == initial_thread_tcb)
- munmap(p, tls_allocation);
+ munmap(p, tls_allocation + sizeof(*tcb));
else
free(p);
}
CVS commit: src/lib/libc/tls
Module Name:src Committed By: joerg Date: Thu Nov 7 22:25:22 UTC 2019 Modified Files: src/lib/libc/tls: tls.c Log Message: Mirror the ld.elf_so logic for handling aligning the TLS size. Most noticable, recompute the start of the TLS area for variant I relative to the TCB. This makes a difference when the segment size and base alignment don't agree. To generate a diff of this commit: cvs rdiff -u -r1.11 -r1.12 src/lib/libc/tls/tls.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files.
CVS commit: src/sys/dev/pci
Module Name:src Committed By: nisimura Date: Thu Nov 7 22:00:37 UTC 2019 Modified Files: src/sys/dev/pci: if_kse.c Log Message: comment touchup To generate a diff of this commit: cvs rdiff -u -r1.40 -r1.41 src/sys/dev/pci/if_kse.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files. Modified files: Index: src/sys/dev/pci/if_kse.c diff -u src/sys/dev/pci/if_kse.c:1.40 src/sys/dev/pci/if_kse.c:1.41 --- src/sys/dev/pci/if_kse.c:1.40 Thu Nov 7 09:05:29 2019 +++ src/sys/dev/pci/if_kse.c Thu Nov 7 22:00:37 2019 @@ -1,4 +1,4 @@ -/* $NetBSD: if_kse.c,v 1.40 2019/11/07 09:05:29 nisimura Exp $ */ +/* $NetBSD: if_kse.c,v 1.41 2019/11/07 22:00:37 nisimura Exp $ */ /*- * Copyright (c) 2006 The NetBSD Foundation, Inc. @@ -29,8 +29,12 @@ * POSSIBILITY OF SUCH DAMAGE. */ +/* + * Micrel 8841/8842 10/100 ethernet driver + */ + #include -__KERNEL_RCSID(0, "$NetBSD: if_kse.c,v 1.40 2019/11/07 09:05:29 nisimura Exp $"); +__KERNEL_RCSID(0, "$NetBSD: if_kse.c,v 1.41 2019/11/07 22:00:37 nisimura Exp $"); #include #include @@ -519,7 +523,7 @@ kse_attach(device_t parent, device_t sel * 8842 MAC is tied with a builtin 3 port switch. * It can do rate control over either of tx / rx direction * respectively, tough, this driver leaves the rate unlimited - * intending 100Mbps maxinum. + * intending 100Mbps maximum. * 2 ports behave in AN mode and this driver provides no mean * to see the exact details. */ @@ -543,7 +547,7 @@ kse_attach(device_t parent, device_t sel IFQ_SET_READY(>if_snd); /* - * KSZ8842 can handle 802.1Q VLAN-sized frames, + * capable of 802.1Q VLAN-sized frames, * can do IPv4, TCPv4, and UDPv4 checksums in hardware. */ sc->sc_ethercom.ec_capabilities |= ETHERCAP_VLAN_MTU;
CVS commit: src/sys/dev/pci
Module Name:src Committed By: nisimura Date: Thu Nov 7 22:00:37 UTC 2019 Modified Files: src/sys/dev/pci: if_kse.c Log Message: comment touchup To generate a diff of this commit: cvs rdiff -u -r1.40 -r1.41 src/sys/dev/pci/if_kse.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files.
CVS commit: src/sys/kern
Module Name:src Committed By: kamil Date: Thu Nov 7 20:34:29 UTC 2019 Modified Files: src/sys/kern: subr_disk_mbr.c Log Message: Revert subr_disk_mbr.c r.1.54 Requested by as there can be a better way to fix the original problem with alignment. To generate a diff of this commit: cvs rdiff -u -r1.55 -r1.56 src/sys/kern/subr_disk_mbr.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files.
CVS commit: src/sys/kern
Module Name:src
Committed By: kamil
Date: Thu Nov 7 20:34:29 UTC 2019
Modified Files:
src/sys/kern: subr_disk_mbr.c
Log Message:
Revert subr_disk_mbr.c r.1.54
Requested by as there can be a better way to fix the original
problem with alignment.
To generate a diff of this commit:
cvs rdiff -u -r1.55 -r1.56 src/sys/kern/subr_disk_mbr.c
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
Modified files:
Index: src/sys/kern/subr_disk_mbr.c
diff -u src/sys/kern/subr_disk_mbr.c:1.55 src/sys/kern/subr_disk_mbr.c:1.56
--- src/sys/kern/subr_disk_mbr.c:1.55 Thu Nov 7 20:30:49 2019
+++ src/sys/kern/subr_disk_mbr.c Thu Nov 7 20:34:29 2019
@@ -1,4 +1,4 @@
-/* $NetBSD: subr_disk_mbr.c,v 1.55 2019/11/07 20:30:49 kamil Exp $ */
+/* $NetBSD: subr_disk_mbr.c,v 1.56 2019/11/07 20:34:29 kamil Exp $ */
/*
* Copyright (c) 1982, 1986, 1988 Regents of the University of California.
@@ -54,7 +54,7 @@
*/
#include
-__KERNEL_RCSID(0, "$NetBSD: subr_disk_mbr.c,v 1.55 2019/11/07 20:30:49 kamil Exp $");
+__KERNEL_RCSID(0, "$NetBSD: subr_disk_mbr.c,v 1.56 2019/11/07 20:34:29 kamil Exp $");
#include
#include
@@ -586,7 +586,7 @@ check_label_magic(const struct disklabel
static int
validate_label(mbr_args_t *a, uint label_sector)
{
- struct disklabel *dlp, tlp;
+ struct disklabel *dlp;
char *dlp_lim, *dlp_byte;
int error;
#ifdef DISKLABEL_EI
@@ -622,23 +622,21 @@ validate_label(mbr_args_t *a, uint label
else
dlp_byte += LABELSECTOR * a->lp->d_secsize;
dlp = (void *)dlp_byte;
- memcpy(, dlp, sizeof(tlp));
break;
}
- memcpy(, dlp, sizeof(tlp));
- if (!check_label_magic(, DISKMAGIC))
+ if (!check_label_magic(dlp, DISKMAGIC))
#ifdef DISKLABEL_EI
{
- if (!check_label_magic(, bswap32(DISKMAGIC)))
+ if (!check_label_magic(dlp, bswap32(DISKMAGIC)))
continue;
/*
* The label is in the other byte order. We need to
* checksum before swapping the byte order.
*/
- npartitions = bswap16(tlp.d_npartitions);
+ npartitions = bswap16(dlp->d_npartitions);
if (npartitions > MAXPARTITIONS ||
- dkcksum_sized(, npartitions) != 0)
+ dkcksum_sized(dlp, npartitions) != 0)
goto corrupted;
swapped = 1;
@@ -646,8 +644,8 @@ validate_label(mbr_args_t *a, uint label
#else
continue;
#endif
- else if (tlp.d_npartitions > MAXPARTITIONS ||
- dkcksum() != 0) {
+ else if (dlp->d_npartitions > MAXPARTITIONS ||
+ dkcksum(dlp) != 0) {
#ifdef DISKLABEL_EI
corrupted:
#endif
@@ -661,11 +659,11 @@ corrupted:
case READ_LABEL:
#ifdef DISKLABEL_EI
if (swapped)
- swap_disklabel(a->lp, );
+ swap_disklabel(a->lp, dlp);
else
- *a->lp = tlp;
+ *a->lp = *dlp;
#else
- *a->lp = tlp;
+ *a->lp = *dlp;
#endif
if ((a->msg = convertdisklabel(a->lp, a->strat, a->bp,
a->secperunit)) != NULL)
@@ -677,13 +675,12 @@ corrupted:
#ifdef DISKLABEL_EI
/* DO NOT swap a->lp itself for later references. */
if (swapped)
- swap_disklabel(, a->lp);
+ swap_disklabel(dlp, a->lp);
else
- tlp = *a->lp;
+ *dlp = *a->lp;
#else
- tlp = *a->lp;
+ *dlp = *a->lp;
#endif
- memcpy(dlp, , sizeof(tlp));
a->bp->b_oflags &= ~BO_DONE;
a->bp->b_flags &= ~B_READ;
a->bp->b_flags |= B_WRITE;
CVS commit: src/sys/kern
Module Name:src Committed By: kamil Date: Thu Nov 7 20:30:49 UTC 2019 Modified Files: src/sys/kern: subr_disk_mbr.c Log Message: Decorate check_label_magic() with __noubsan Requested by To generate a diff of this commit: cvs rdiff -u -r1.54 -r1.55 src/sys/kern/subr_disk_mbr.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files.
CVS commit: src/sys/kern
Module Name:src
Committed By: kamil
Date: Thu Nov 7 20:30:49 UTC 2019
Modified Files:
src/sys/kern: subr_disk_mbr.c
Log Message:
Decorate check_label_magic() with __noubsan
Requested by
To generate a diff of this commit:
cvs rdiff -u -r1.54 -r1.55 src/sys/kern/subr_disk_mbr.c
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
Modified files:
Index: src/sys/kern/subr_disk_mbr.c
diff -u src/sys/kern/subr_disk_mbr.c:1.54 src/sys/kern/subr_disk_mbr.c:1.55
--- src/sys/kern/subr_disk_mbr.c:1.54 Thu Nov 7 18:35:41 2019
+++ src/sys/kern/subr_disk_mbr.c Thu Nov 7 20:30:49 2019
@@ -1,4 +1,4 @@
-/* $NetBSD: subr_disk_mbr.c,v 1.54 2019/11/07 18:35:41 kamil Exp $ */
+/* $NetBSD: subr_disk_mbr.c,v 1.55 2019/11/07 20:30:49 kamil Exp $ */
/*
* Copyright (c) 1982, 1986, 1988 Regents of the University of California.
@@ -54,7 +54,7 @@
*/
#include
-__KERNEL_RCSID(0, "$NetBSD: subr_disk_mbr.c,v 1.54 2019/11/07 18:35:41 kamil Exp $");
+__KERNEL_RCSID(0, "$NetBSD: subr_disk_mbr.c,v 1.55 2019/11/07 20:30:49 kamil Exp $");
#include
#include
@@ -568,6 +568,7 @@ look_netbsd_part(mbr_args_t *a, mbr_part
return SCAN_CONTINUE;
}
+__noubsan
static bool
check_label_magic(const struct disklabel *dlp, uint32_t diskmagic)
{
CVS commit: src/sys/kern
Module Name:src Committed By: joerg Date: Thu Nov 7 19:45:18 UTC 2019 Modified Files: src/sys/kern: kern_lwp.c Log Message: Preserve the LWP ID of the calling thread on (v)fork. This ensures that _lwp_self() remains invariant as necessary for the locking in the dynamic linker. Otherwise if a process creates a thread and forks from it, the main thread of the parent would share the LWP ID of the main thread of the child, even though they have different origins. Partial fix for pkg/54192. To generate a diff of this commit: cvs rdiff -u -r1.205 -r1.206 src/sys/kern/kern_lwp.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files.
CVS commit: src/sys/kern
Module Name:src
Committed By: joerg
Date: Thu Nov 7 19:45:18 UTC 2019
Modified Files:
src/sys/kern: kern_lwp.c
Log Message:
Preserve the LWP ID of the calling thread on (v)fork. This ensures that
_lwp_self() remains invariant as necessary for the locking in the
dynamic linker. Otherwise if a process creates a thread and forks from
it, the main thread of the parent would share the LWP ID of the main
thread of the child, even though they have different origins.
Partial fix for pkg/54192.
To generate a diff of this commit:
cvs rdiff -u -r1.205 -r1.206 src/sys/kern/kern_lwp.c
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
Modified files:
Index: src/sys/kern/kern_lwp.c
diff -u src/sys/kern/kern_lwp.c:1.205 src/sys/kern/kern_lwp.c:1.206
--- src/sys/kern/kern_lwp.c:1.205 Sun Oct 6 15:11:17 2019
+++ src/sys/kern/kern_lwp.c Thu Nov 7 19:45:18 2019
@@ -1,4 +1,4 @@
-/* $NetBSD: kern_lwp.c,v 1.205 2019/10/06 15:11:17 uwe Exp $ */
+/* $NetBSD: kern_lwp.c,v 1.206 2019/11/07 19:45:18 joerg Exp $ */
/*-
* Copyright (c) 2001, 2006, 2007, 2008, 2009 The NetBSD Foundation, Inc.
@@ -211,7 +211,7 @@
*/
#include
-__KERNEL_RCSID(0, "$NetBSD: kern_lwp.c,v 1.205 2019/10/06 15:11:17 uwe Exp $");
+__KERNEL_RCSID(0, "$NetBSD: kern_lwp.c,v 1.206 2019/11/07 19:45:18 joerg Exp $");
#include "opt_ddb.h"
#include "opt_lockdebug.h"
@@ -902,6 +902,8 @@ lwp_create(lwp_t *l1, proc_t *p2, vaddr_
if ((flags & LWP_PIDLID) != 0) {
lid = proc_alloc_pid(p2);
l2->l_pflag |= LP_PIDLID;
+ } else if (p2->p_nlwps == 0) {
+ lid = l1->l_lid;
} else {
lid = 0;
}
Re: CVS commit: src/sys/kern
On Nov 7, 6:08pm, n...@gmx.com (Kamil Rytarowski) wrote: -- Subject: Re: CVS commit: src/sys/kern | Please review: | | http://netbsd.org/~kamil/patch-00194-disklabel-alignment.txt | | This patch works for me. | | Patch inspired by: | | Avoid misaligned access in disklabel(8) in find_label() | https://github.com/NetBSD/src/commit/19bd3d170c8fd67052ca4ca20151cd77d893ab= | 88 | | After landing it I will revert "Avoid unaligned pointer arithmetic in | check_label_magic()". Yeah, let's do that. christos
Re: CVS commit: src/sys/kern
On Thu, Nov 07, 2019 at 19:06:29 +0100, Martin Husemann wrote: > OK, why is it 8 byte aligned? Checking > > > revision 1.108 > > date: 2011-01-18 20:52:24 +0100; author: matt; state: Exp; lines: +2 -1; > > Make struct disklabel 8 byte aligned. This increases its size by 4 bytes > > on IPL32 platforms so add code in sys_ioctl (and netbsd32_ioctl) to deal > > with the older/smaller diskabel size. This change makes disklabel the > > same for both IPL32 and LP64 platforms. > > Argh! Somehow I dimly remember having been here already ... > I think I proposed to change the loop to 8 byte increments back then, but > noone knows what odd systems it will the stop working on (none at all is my > personal bet). I gather the alignment mess is caused by pointers d_un.un_b.un_d_boot0 and d_un.un_b.un_d_boot1 that are inside the d_un union and are NOT part of the actual disklabel: "These are returned when using getdiskbyname(3) to retrieve the values from /etc/disktab." So that was completely self-inflicted. The loop is fine for the on-disk structure. Now the question is how to dig our way out of this, b/c unfortunately it's a public interface. But the mindless memcpy should be the last resort IMO. -uwe
Re: CVS commit: src/sys/kern
On 07.11.2019 19:32, Valery Ushakov wrote: > On Thu, Nov 07, 2019 at 18:08:40 +0100, Kamil Rytarowski wrote: > >> On 07.11.2019 16:45, Kamil Rytarowski wrote: >>> On 07.11.2019 16:26, Martin Husemann wrote: On Thu, Nov 07, 2019 at 02:53:08PM +0100, Kamil Rytarowski wrote: > On 07.11.2019 14:25, Valery Ushakov wrote: >> If the sanitizer does complain about other uses, there is little point >> in fixing one instance and not the others. > > We already agreed with Christos that this is appeasing of GCC. If you > want to scan the whole kernel (or whole C) file for more occurrences of > violations - please go for it. No. The commit needs to be reverted, and then a) either the root cause for the unaligned address be fixed or b) some other means be found to make the sanitizer shut up As uwe said: papering over a tiny detail that *never* hits in the real world but potentialy hiding a real issue is not the way to go. >>> >>> I don't have a readily available reproducer locally but it was breaking >>> syzbot from booting after the switch to gcc8. I will fix it differently >>> aligning the whole struct (so the same approach as we use in userland) >>> and backout this change. >>> >> >> Please review: >> >> http://netbsd.org/~kamil/patch-00194-disklabel-alignment.txt >> >> This patch works for me. > > What happens if you change check_label_magic() to use direct member > accesses (as the code did before xtos change it) instead of memcmp? > Does that shup up the sanitizer? I assume it should as it doesn't > complain about other member accesses. I'd strongly prefer this change > for now. > > -uwe > I have got no opinion on this. It will work now. If you prefer it, please go for it. signature.asc Description: OpenPGP digital signature
CVS commit: src/sys/kern
Module Name:src Committed By: kamil Date: Thu Nov 7 18:35:41 UTC 2019 Modified Files: src/sys/kern: subr_disk_mbr.c Log Message: Revert src/sys/kern/subr_disk_mbr.c r.1.52 Addressed in a better way in r. 1.53. To generate a diff of this commit: cvs rdiff -u -r1.53 -r1.54 src/sys/kern/subr_disk_mbr.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files.
CVS commit: src/sys/kern
Module Name:src
Committed By: kamil
Date: Thu Nov 7 18:35:41 UTC 2019
Modified Files:
src/sys/kern: subr_disk_mbr.c
Log Message:
Revert src/sys/kern/subr_disk_mbr.c r.1.52
Addressed in a better way in r. 1.53.
To generate a diff of this commit:
cvs rdiff -u -r1.53 -r1.54 src/sys/kern/subr_disk_mbr.c
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
Modified files:
Index: src/sys/kern/subr_disk_mbr.c
diff -u src/sys/kern/subr_disk_mbr.c:1.53 src/sys/kern/subr_disk_mbr.c:1.54
--- src/sys/kern/subr_disk_mbr.c:1.53 Thu Nov 7 18:30:27 2019
+++ src/sys/kern/subr_disk_mbr.c Thu Nov 7 18:35:41 2019
@@ -1,4 +1,4 @@
-/* $NetBSD: subr_disk_mbr.c,v 1.53 2019/11/07 18:30:27 kamil Exp $ */
+/* $NetBSD: subr_disk_mbr.c,v 1.54 2019/11/07 18:35:41 kamil Exp $ */
/*
* Copyright (c) 1982, 1986, 1988 Regents of the University of California.
@@ -54,7 +54,7 @@
*/
#include
-__KERNEL_RCSID(0, "$NetBSD: subr_disk_mbr.c,v 1.53 2019/11/07 18:30:27 kamil Exp $");
+__KERNEL_RCSID(0, "$NetBSD: subr_disk_mbr.c,v 1.54 2019/11/07 18:35:41 kamil Exp $");
#include
#include
@@ -571,11 +571,8 @@ look_netbsd_part(mbr_args_t *a, mbr_part
static bool
check_label_magic(const struct disklabel *dlp, uint32_t diskmagic)
{
-
- return memcmp((const char *)dlp + offsetof(struct disklabel, d_magic),
- , sizeof(diskmagic)) == 0 &&
- memcmp((const char *)dlp + offsetof(struct disklabel, d_magic2),
- , sizeof(diskmagic)) == 0;
+ return memcmp(>d_magic, , sizeof(diskmagic)) == 0 &&
+ memcmp(>d_magic2, , sizeof(diskmagic)) == 0;
}
#ifdef DISKLABEL_EI
Re: CVS commit: src/sys/kern
On Thu, Nov 07, 2019 at 18:08:40 +0100, Kamil Rytarowski wrote: > On 07.11.2019 16:45, Kamil Rytarowski wrote: > > On 07.11.2019 16:26, Martin Husemann wrote: > >> On Thu, Nov 07, 2019 at 02:53:08PM +0100, Kamil Rytarowski wrote: > >>> On 07.11.2019 14:25, Valery Ushakov wrote: > If the sanitizer does complain about other uses, there is little point > in fixing one instance and not the others. > >>> > >>> We already agreed with Christos that this is appeasing of GCC. If you > >>> want to scan the whole kernel (or whole C) file for more occurrences of > >>> violations - please go for it. > >> > >> No. The commit needs to be reverted, and then > >> > >> a) either the root cause for the unaligned address be fixed or > >> b) some other means be found to make the sanitizer shut up > >> > >> As uwe said: papering over a tiny detail that *never* hits in the real > >> world but potentialy hiding a real issue is not the way to go. > >> > > > > I don't have a readily available reproducer locally but it was breaking > > syzbot from booting after the switch to gcc8. I will fix it differently > > aligning the whole struct (so the same approach as we use in userland) > > and backout this change. > > > > Please review: > > http://netbsd.org/~kamil/patch-00194-disklabel-alignment.txt > > This patch works for me. What happens if you change check_label_magic() to use direct member accesses (as the code did before xtos change it) instead of memcmp? Does that shup up the sanitizer? I assume it should as it doesn't complain about other member accesses. I'd strongly prefer this change for now. -uwe
CVS commit: src/sys/kern
Module Name:src Committed By: kamil Date: Thu Nov 7 18:30:27 UTC 2019 Modified Files: src/sys/kern: subr_disk_mbr.c Log Message: Ensure in validate_label() that struct disklabel pointer is 8-byte aligned The label is searched each 4 bytes and can be detected in an unaligned location. Before any operations on it, copy it to promptly aligned local copy on the stack. This is a missing part of the following change: revision 1.108 date: 2011-01-18 20:52:24 +0100; author: matt; state: Exp; lines: +2 -1; Make struct disklabel 8 byte aligned. This increases its size by 4 bytes on IPL32 platforms so add code in sys_ioctl (and netbsd32_ioctl) to deal with the older/smaller diskabel size. This change makes disklabel the same for both IPL32 and LP64 platforms OK by To generate a diff of this commit: cvs rdiff -u -r1.52 -r1.53 src/sys/kern/subr_disk_mbr.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files.
CVS commit: src/sys/kern
Module Name:src
Committed By: kamil
Date: Thu Nov 7 18:30:27 UTC 2019
Modified Files:
src/sys/kern: subr_disk_mbr.c
Log Message:
Ensure in validate_label() that struct disklabel pointer is 8-byte aligned
The label is searched each 4 bytes and can be detected in an unaligned
location. Before any operations on it, copy it to promptly aligned local
copy on the stack.
This is a missing part of the following change:
revision 1.108
date: 2011-01-18 20:52:24 +0100; author: matt; state: Exp; lines: +2 -1;
Make struct disklabel 8 byte aligned. This increases its size by 4 bytes
on IPL32 platforms so add code in sys_ioctl (and netbsd32_ioctl) to deal
with the older/smaller diskabel size. This change makes disklabel the
same for both IPL32 and LP64 platforms
OK by
To generate a diff of this commit:
cvs rdiff -u -r1.52 -r1.53 src/sys/kern/subr_disk_mbr.c
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
Modified files:
Index: src/sys/kern/subr_disk_mbr.c
diff -u src/sys/kern/subr_disk_mbr.c:1.52 src/sys/kern/subr_disk_mbr.c:1.53
--- src/sys/kern/subr_disk_mbr.c:1.52 Wed Nov 6 13:07:32 2019
+++ src/sys/kern/subr_disk_mbr.c Thu Nov 7 18:30:27 2019
@@ -1,4 +1,4 @@
-/* $NetBSD: subr_disk_mbr.c,v 1.52 2019/11/06 13:07:32 kamil Exp $ */
+/* $NetBSD: subr_disk_mbr.c,v 1.53 2019/11/07 18:30:27 kamil Exp $ */
/*
* Copyright (c) 1982, 1986, 1988 Regents of the University of California.
@@ -54,7 +54,7 @@
*/
#include
-__KERNEL_RCSID(0, "$NetBSD: subr_disk_mbr.c,v 1.52 2019/11/06 13:07:32 kamil Exp $");
+__KERNEL_RCSID(0, "$NetBSD: subr_disk_mbr.c,v 1.53 2019/11/07 18:30:27 kamil Exp $");
#include
#include
@@ -588,7 +588,7 @@ check_label_magic(const struct disklabel
static int
validate_label(mbr_args_t *a, uint label_sector)
{
- struct disklabel *dlp;
+ struct disklabel *dlp, tlp;
char *dlp_lim, *dlp_byte;
int error;
#ifdef DISKLABEL_EI
@@ -624,21 +624,23 @@ validate_label(mbr_args_t *a, uint label
else
dlp_byte += LABELSECTOR * a->lp->d_secsize;
dlp = (void *)dlp_byte;
+ memcpy(, dlp, sizeof(tlp));
break;
}
- if (!check_label_magic(dlp, DISKMAGIC))
+ memcpy(, dlp, sizeof(tlp));
+ if (!check_label_magic(, DISKMAGIC))
#ifdef DISKLABEL_EI
{
- if (!check_label_magic(dlp, bswap32(DISKMAGIC)))
+ if (!check_label_magic(, bswap32(DISKMAGIC)))
continue;
/*
* The label is in the other byte order. We need to
* checksum before swapping the byte order.
*/
- npartitions = bswap16(dlp->d_npartitions);
+ npartitions = bswap16(tlp.d_npartitions);
if (npartitions > MAXPARTITIONS ||
- dkcksum_sized(dlp, npartitions) != 0)
+ dkcksum_sized(, npartitions) != 0)
goto corrupted;
swapped = 1;
@@ -646,8 +648,8 @@ validate_label(mbr_args_t *a, uint label
#else
continue;
#endif
- else if (dlp->d_npartitions > MAXPARTITIONS ||
- dkcksum(dlp) != 0) {
+ else if (tlp.d_npartitions > MAXPARTITIONS ||
+ dkcksum() != 0) {
#ifdef DISKLABEL_EI
corrupted:
#endif
@@ -661,11 +663,11 @@ corrupted:
case READ_LABEL:
#ifdef DISKLABEL_EI
if (swapped)
- swap_disklabel(a->lp, dlp);
+ swap_disklabel(a->lp, );
else
- *a->lp = *dlp;
+ *a->lp = tlp;
#else
- *a->lp = *dlp;
+ *a->lp = tlp;
#endif
if ((a->msg = convertdisklabel(a->lp, a->strat, a->bp,
a->secperunit)) != NULL)
@@ -677,12 +679,13 @@ corrupted:
#ifdef DISKLABEL_EI
/* DO NOT swap a->lp itself for later references. */
if (swapped)
- swap_disklabel(dlp, a->lp);
+ swap_disklabel(, a->lp);
else
- *dlp = *a->lp;
+ tlp = *a->lp;
#else
- *dlp = *a->lp;
+ tlp = *a->lp;
#endif
+ memcpy(dlp, , sizeof(tlp));
a->bp->b_oflags &= ~BO_DONE;
a->bp->b_flags &= ~B_READ;
a->bp->b_flags |= B_WRITE;
Re: CVS commit: src/sys/kern
On Thu, Nov 07, 2019 at 06:46:48PM +0100, Kamil Rytarowski wrote: > member access within misaligned address 0x942d3de8c03c for type > 'const struct disklabel' which requires 8 byte alignment OK, why is it 8 byte aligned? Checking > revision 1.108 > date: 2011-01-18 20:52:24 +0100; author: matt; state: Exp; lines: +2 -1; > Make struct disklabel 8 byte aligned. This increases its size by 4 bytes > on IPL32 platforms so add code in sys_ioctl (and netbsd32_ioctl) to deal > with the older/smaller diskabel size. This change makes disklabel the > same for both IPL32 and LP64 platforms. Argh! Somehow I dimly remember having been here already ... I think I proposed to change the loop to 8 byte increments back then, but noone knows what odd systems it will the stop working on (none at all is my personal bet). So I guess your patch is the best way forward. Martin
Re: CVS commit: src/sys/kern
On 07.11.2019 18:20, Martin Husemann wrote:
> On Thu, Nov 07, 2019 at 06:08:40PM +0100, Kamil Rytarowski wrote:
>> Please review:
>>
>> http://netbsd.org/~kamil/patch-00194-disklabel-alignment.txt
>>
>> This patch works for me.
>
> Yes, I believe that it does - but why is it needed?
>
syzbot reported on boot:
member access within misaligned address 0x942d3de8c03c for type
'const struct disklabel' which requires 8 byte alignment
> dlp = (void *)a->bp->b_data;
>
> Here we can assume that b_data is properly aligned (likely even 512 byte
> aligned or similar).
>
I just have got this backtrace:
[ 1.3496393] panic: UBSan: Undefined Behavior in
/syzkaller/managers/netbsd-kubsan/kernel/sys/kern/subr_disk_mbr.c:574:9,
member access within misaligned address 0x942d3de8c03c for type
'const struct disklabel' which requires 8 byte alignment
[ 1.3663828] cpu0: Begin traceback...
[ 1.3689943] vpanic() at netbsd:vpanic+0x2aa sys/kern/subr_prf.c:336
[ 1.3788101] isAlreadyReported() at netbsd:isAlreadyReported
[ 1.3988418] HandleTypeMismatch.part.1() at
netbsd:HandleTypeMismatch.part.1+0xcc
[ 1.4088591] HandleTypeMismatch() at netbsd:HandleTypeMismatch+0x7b
sys/../common/lib/libc/misc/ubsan.c:408
[ 1.4288896] check_label_magic() at netbsd:check_label_magic+0x9f
sys/kern/subr_disk_mbr.c:574
[ 1.4389098] validate_label() at netbsd:validate_label+0x1b4
sys/kern/subr_disk_mbr.c:626
[ 1.4489214] look_netbsd_part() at netbsd:look_netbsd_part+0x3ce
sys/kern/subr_disk_mbr.c:526
[ 1.4689530] scan_mbr() at netbsd:scan_mbr+0x24a
sys/kern/subr_disk_mbr.c:234
[ 1.4789689] readdisklabel() at netbsd:readdisklabel+0x412
sys/kern/subr_disk_mbr.c:448
[ 1.4898443] dk_getdisklabel() at netbsd:dk_getdisklabel+0x192
sys/dev/dksubr.c:931
[ 1.5090177] dk_open() at netbsd:dk_open+0x456 sys/dev/dksubr.c:177
[ 1.5190333] sdopen() at netbsd:sdopen+0x114 sys/dev/scsipi/sd.c:543
[ 1.5290486] cdev_open() at netbsd:cdev_open+0xe5
sys/kern/subr_devsw.c:871
[ 1.5490803] spec_open() at netbsd:spec_open+0x3ad
sys/miscfs/specfs/spec_vnops.c:562
[ 1.5591508] VOP_OPEN() at netbsd:VOP_OPEN+0x113 sys/kern/vnode_if.c:298
[ 1.5791291] dkwedge_discover() at netbsd:dkwedge_discover+0xcf
sys/dev/dkwedge/dk.c:931
[ 1.5891449] sdattach() at netbsd:sdattach+0x53f sys/dev/scsipi/sd.c:362
[ 1.5991611] config_attach_loc() at netbsd:config_attach_loc+0x432
sys/kern/subr_autoconf.c:1604
[ 1.6191925] scsi_probe_bus() at netbsd:scsi_probe_bus+0xad8
scsi_probe_device sys/dev/scsipi/scsiconf.c:1035 [inline]
[ 1.6191925] scsi_probe_bus() at netbsd:scsi_probe_bus+0xad8
sys/dev/scsipi/scsiconf.c:413
[ 1.6292092] scsibus_discover_thread() at
netbsd:scsibus_discover_thread+0xdd scsibus_config
sys/dev/scsipi/scsiconf.c:320 [inline]
[ 1.6292092] scsibus_discover_thread() at
netbsd:scsibus_discover_thread+0xdd sys/dev/scsipi/scsiconf.c:285
[ 1.6392253] cpu0: End traceback...
[ 1.6392253] fatal breakpoint trap in supervisor mode
[ 1.6392253] trap type 1 code 0 rip 0x8021 cs 0x8 rflags
0x282 cr2 0 ilevel 0 rsp 0xbc80a5c02280
[ 1.6555812] curlwp 0x942c2bcce9c0 pid 0.28 lowest kstack
0xbc80a5bff2c0
https://syzkaller.appspot.com/bug?extid=56769dece0ec3e35731e
> for (;; dlp = (void *)((char *)dlp + sizeof(uint32_t))) {
>
> ugly as it is written, this still should ensure proper (4 byte) alignement
> of all later values of dlp.
We actually need 8-byte alignment to make a compiler appeased.. so this
patch is still good (if I remember correctly it was the original
motivation for patching the userland part).
Can I land it?
>
> Do we have any architectures with odd LABELOFFSET?
> I only found 0, 64, 128 and 516, none of those should cause alignment
> issues in the code following.
>
> So where is the problem?
>
Could that be a half-corrupted image after some crash? The previous
patch actually fixed booting.. so I don't know.
> Martin
>
signature.asc
Description: OpenPGP digital signature
Re: CVS commit: src/sys/kern
On Thu, Nov 07, 2019 at 06:08:40PM +0100, Kamil Rytarowski wrote:
> Please review:
>
> http://netbsd.org/~kamil/patch-00194-disklabel-alignment.txt
>
> This patch works for me.
Yes, I believe that it does - but why is it needed?
dlp = (void *)a->bp->b_data;
Here we can assume that b_data is properly aligned (likely even 512 byte
aligned or similar).
for (;; dlp = (void *)((char *)dlp + sizeof(uint32_t))) {
ugly as it is written, this still should ensure proper (4 byte) alignement
of all later values of dlp.
Do we have any architectures with odd LABELOFFSET?
I only found 0, 64, 128 and 516, none of those should cause alignment
issues in the code following.
So where is the problem?
Martin
Re: CVS commit: src/sys/kern
On 07.11.2019 16:45, Kamil Rytarowski wrote: > On 07.11.2019 16:26, Martin Husemann wrote: >> On Thu, Nov 07, 2019 at 02:53:08PM +0100, Kamil Rytarowski wrote: >>> On 07.11.2019 14:25, Valery Ushakov wrote: If the sanitizer does complain about other uses, there is little point in fixing one instance and not the others. >>> >>> We already agreed with Christos that this is appeasing of GCC. If you >>> want to scan the whole kernel (or whole C) file for more occurrences of >>> violations - please go for it. >> >> No. The commit needs to be reverted, and then >> >> a) either the root cause for the unaligned address be fixed or >> b) some other means be found to make the sanitizer shut up >> >> As uwe said: papering over a tiny detail that *never* hits in the real >> world but potentialy hiding a real issue is not the way to go. >> > > I don't have a readily available reproducer locally but it was breaking > syzbot from booting after the switch to gcc8. I will fix it differently > aligning the whole struct (so the same approach as we use in userland) > and backout this change. > Please review: http://netbsd.org/~kamil/patch-00194-disklabel-alignment.txt This patch works for me. Patch inspired by: Avoid misaligned access in disklabel(8) in find_label() https://github.com/NetBSD/src/commit/19bd3d170c8fd67052ca4ca20151cd77d893ab88 After landing it I will revert "Avoid unaligned pointer arithmetic in check_label_magic()". signature.asc Description: OpenPGP digital signature
Re: CVS commit: src/sys/kern
David Young wrote in <20191107155806.gl1...@pobox.com>: |On Thu, Nov 07, 2019 at 04:26:51PM +0100, Martin Husemann wrote: |> On Thu, Nov 07, 2019 at 02:53:08PM +0100, Kamil Rytarowski wrote: |>> On 07.11.2019 14:25, Valery Ushakov wrote: .. |I think the problem is that if you have the series of statements, | |element_t *e = >element; | |if (s == NULL) |return; ... |There is probably an argument to be made that in a |segmented/tagged/capability architecture that has run C programs |(8086; Burroughs Large Systems) or may run them in the future (CHERI), |&(NULL)->element cannot sensibly be computed. You mean they will render any non-compiler-builtin approach to create field_offsetof() and field_sizeof() impossible. --steffen | |Der Kragenbaer,The moon bear, |der holt sich munter he cheerfully and one by one |einen nach dem anderen runter wa.ks himself off |(By Robert Gernhardt)
Re: CVS commit: src/sys/kern
On 07.11.2019 17:20, Kamil Rytarowski wrote: > On 07.11.2019 17:08, Martin Husemann wrote: >> On Thu, Nov 07, 2019 at 04:56:16PM +0100, Kamil Rytarowski wrote: >>> 6.3.2.1 C11 >>> >>> 'An lvalue is an expression (with an object type other than void) that >>> potentially designates an object' >>> >>> This means that real dereference is not needed, only a potential. And >>> there are special cases of pointer arithmetic. >> >> But 6.5.3.2 says that taking the address of a pointer expression does >> not "evaluate the * oparator", but instead is shortcut to address addition. >> > > I think that this does not apply as & is applied already to lvalue. This > part of standard specifies &*p, not something like &((*p).x). > To be clear the third case (next to applying to * and lvalue) is applying & to the result of []. Unfortunately p->x goes into the lvalue section. This is how I interpret it (and find it as unreasonable in practice). >> The annotations talk about various special cases and not the concreate one >> at hand, but in my reading the intention is clear and this part should be >> clarified. >> > > I can agree that this would be nicer to be defined, unfortunately I > think that we can merely ask for exception to apply & to -> operator. > >> Martin >> > > signature.asc Description: OpenPGP digital signature
Re: CVS commit: src/sys/kern
On Thu, Nov 07, 2019 at 09:58:06 -0600, David Young wrote: > On Thu, Nov 07, 2019 at 04:26:51PM +0100, Martin Husemann wrote: > > On Thu, Nov 07, 2019 at 02:53:08PM +0100, Kamil Rytarowski wrote: > > > On 07.11.2019 14:25, Valery Ushakov wrote: > > > > If the sanitizer does complain about other uses, there is little point > > > > in fixing one instance and not the others. > > > > > > We already agreed with Christos that this is appeasing of GCC. If you > > > want to scan the whole kernel (or whole C) file for more occurrences of > > > violations - please go for it. > > > > No. The commit needs to be reverted, and then > > > > a) either the root cause for the unaligned address be fixed or > > b) some other means be found to make the sanitizer shut up > > > > As uwe said: papering over a tiny detail that *never* hits in the real > > world but potentialy hiding a real issue is not the way to go. > > > > Martin > > > > P.S.: Independend of this I would still like an official C standard > > clarification; in my reading a simple address calculation is not > > accessing an object through a pointer (which would be the undefined > > behaviour). If the C standard is not clear on this, it needs to be > > improved. > > I think the problem is that if you have the series of statements, > > element_t *e = >element; > > if (s == NULL) > return; > > then the comparison with NULL implies that in this scope, s could > be NULL. NULL does not necessarily have any "arithmetic" relationship > to any other pointer---by that rationale, you probably cannot assign > any alignment to it---so there is no sensible value that you can > give to e. This is not what the changed code does. The code in question has struct disklabel *dlp = ...; apparently gcc complains about memcmp(>d_magic, ...) but later the code uses e.g. dlp->d_partitions (right after the check_label_magic call) and other memebers. So it's very suspicious that one usage is flagged and others are not. Until very recently the magic check was also explicitly comparing dlp->d_magic != DISKMAGIC, etc. So may be we should stop pretending and rewrite check_label_magic() to use that instead of memcmp. (And then fix all dlp->foo in one swoop). If my I interpretation is wrong, I would be glad to be corrected. > There is probably an argument to be made that in a > segmented/tagged/capability architecture that has run C programs > (8086; Burroughs Large Systems) or may run them in the future (CHERI), > &(NULL)->element cannot sensibly be computed. Amen :). I actually did encounter problems like that when compiling software on Xenix 286 ages ago (e.g. 0 instead of NULL passed as the last argument to exec). While that is a fascinating excercise, it's not what happens here. -uwe
Re: CVS commit: src/sys/kern
On 07.11.2019 17:09, Martin Husemann wrote: > On Thu, Nov 07, 2019 at 09:58:06AM -0600, David Young wrote: >> I think the problem is that if you have the series of statements, >> >> element_t *e = >element; >> >> if (s == NULL) >> return; > > Note that this example has *nothing* in common with Kamil's code change. > He cited it as an example of the sanitize being usefull, but it only > distracted from the real issue. > > Martin > This is the very similar case, except UB is a different part, in misaligned pointer rather that dereferened NULL. signature.asc Description: OpenPGP digital signature
Re: CVS commit: src/sys/kern
On 07.11.2019 17:08, Martin Husemann wrote: > On Thu, Nov 07, 2019 at 04:56:16PM +0100, Kamil Rytarowski wrote: >> 6.3.2.1 C11 >> >> 'An lvalue is an expression (with an object type other than void) that >> potentially designates an object' >> >> This means that real dereference is not needed, only a potential. And >> there are special cases of pointer arithmetic. > > But 6.5.3.2 says that taking the address of a pointer expression does > not "evaluate the * oparator", but instead is shortcut to address addition. > I think that this does not apply as & is applied already to lvalue. This part of standard specifies &*p, not something like &((*p).x). > The annotations talk about various special cases and not the concreate one > at hand, but in my reading the intention is clear and this part should be > clarified. > I can agree that this would be nicer to be defined, unfortunately I think that we can merely ask for exception to apply & to -> operator. > Martin > signature.asc Description: OpenPGP digital signature
Re: CVS commit: src/sys/kern
On Thu, Nov 07, 2019 at 09:58:06AM -0600, David Young wrote: > I think the problem is that if you have the series of statements, > > element_t *e = >element; > > if (s == NULL) > return; Note that this example has *nothing* in common with Kamil's code change. He cited it as an example of the sanitize being usefull, but it only distracted from the real issue. Martin
Re: CVS commit: src/sys/kern
On Thu, Nov 07, 2019 at 04:56:16PM +0100, Kamil Rytarowski wrote: > 6.3.2.1 C11 > > 'An lvalue is an expression (with an object type other than void) that > potentially designates an object' > > This means that real dereference is not needed, only a potential. And > there are special cases of pointer arithmetic. But 6.5.3.2 says that taking the address of a pointer expression does not "evaluate the * oparator", but instead is shortcut to address addition. The annotations talk about various special cases and not the concreate one at hand, but in my reading the intention is clear and this part should be clarified. Martin
Re: CVS commit: src/sys/kern
On Thu, Nov 07, 2019 at 04:26:51PM +0100, Martin Husemann wrote: > On Thu, Nov 07, 2019 at 02:53:08PM +0100, Kamil Rytarowski wrote: > > On 07.11.2019 14:25, Valery Ushakov wrote: > > > If the sanitizer does complain about other uses, there is little point > > > in fixing one instance and not the others. > > > > We already agreed with Christos that this is appeasing of GCC. If you > > want to scan the whole kernel (or whole C) file for more occurrences of > > violations - please go for it. > > No. The commit needs to be reverted, and then > > a) either the root cause for the unaligned address be fixed or > b) some other means be found to make the sanitizer shut up > > As uwe said: papering over a tiny detail that *never* hits in the real > world but potentialy hiding a real issue is not the way to go. > > Martin > > P.S.: Independend of this I would still like an official C standard > clarification; in my reading a simple address calculation is not > accessing an object through a pointer (which would be the undefined > behaviour). If the C standard is not clear on this, it needs to be > improved. I think the problem is that if you have the series of statements, element_t *e = >element; if (s == NULL) return; then the comparison with NULL implies that in this scope, s could be NULL. NULL does not necessarily have any "arithmetic" relationship to any other pointer---by that rationale, you probably cannot assign any alignment to it---so there is no sensible value that you can give to e. There is probably an argument to be made that in a segmented/tagged/capability architecture that has run C programs (8086; Burroughs Large Systems) or may run them in the future (CHERI), &(NULL)->element cannot sensibly be computed. Dave -- David Young dyo...@pobox.comUrbana, IL(217) 721-9981
Re: CVS commit: src/sys/kern
On 07.11.2019 16:49, Martin Husemann wrote: > On Thu, Nov 07, 2019 at 04:45:31PM +0100, Kamil Rytarowski wrote: >> Unfortunately the C committee went into the opposite direction here and >> specified a potential dereference. > > Where? > > Martin > 6.3.2.1 C99 "An lvalue is an expression with an object type or an incomplete type other than void" 6.3.2.1 C11 'An lvalue is an expression (with an object type other than void) that potentially designates an object' This means that real dereference is not needed, only a potential. And there are special cases of pointer arithmetic. signature.asc Description: OpenPGP digital signature
Re: CVS commit: src/sys/kern
On Thu, Nov 07, 2019 at 04:45:31PM +0100, Kamil Rytarowski wrote: > Unfortunately the C committee went into the opposite direction here and > specified a potential dereference. Where? Martin
Re: CVS commit: src/sys/kern
On 07.11.2019 16:26, Martin Husemann wrote: > On Thu, Nov 07, 2019 at 02:53:08PM +0100, Kamil Rytarowski wrote: >> On 07.11.2019 14:25, Valery Ushakov wrote: >>> If the sanitizer does complain about other uses, there is little point >>> in fixing one instance and not the others. >> >> We already agreed with Christos that this is appeasing of GCC. If you >> want to scan the whole kernel (or whole C) file for more occurrences of >> violations - please go for it. > > No. The commit needs to be reverted, and then > > a) either the root cause for the unaligned address be fixed or > b) some other means be found to make the sanitizer shut up > > As uwe said: papering over a tiny detail that *never* hits in the real > world but potentialy hiding a real issue is not the way to go. > I don't have a readily available reproducer locally but it was breaking syzbot from booting after the switch to gcc8. I will fix it differently aligning the whole struct (so the same approach as we use in userland) and backout this change. > Martin > > P.S.: Independend of this I would still like an official C standard > clarification; in my reading a simple address calculation is not > accessing an object through a pointer (which would be the undefined > behaviour). If the C standard is not clear on this, it needs to be > improved. > Unfortunately the C committee went into the opposite direction here and specified a potential dereference. All I can do now is to add another exception, [x] is allowed and it is not far from >x. It is also a tricky part as some things are unequally documented for p->x and for (*p).x... so not sure if it is worth trying out really... especially that offsetof() as defined for this purpose. signature.asc Description: OpenPGP digital signature
Re: CVS commit: src/sys/kern
On Thu, Nov 07, 2019 at 02:53:08PM +0100, Kamil Rytarowski wrote: > On 07.11.2019 14:25, Valery Ushakov wrote: > > If the sanitizer does complain about other uses, there is little point > > in fixing one instance and not the others. > > We already agreed with Christos that this is appeasing of GCC. If you > want to scan the whole kernel (or whole C) file for more occurrences of > violations - please go for it. No. The commit needs to be reverted, and then a) either the root cause for the unaligned address be fixed or b) some other means be found to make the sanitizer shut up As uwe said: papering over a tiny detail that *never* hits in the real world but potentialy hiding a real issue is not the way to go. Martin P.S.: Independend of this I would still like an official C standard clarification; in my reading a simple address calculation is not accessing an object through a pointer (which would be the undefined behaviour). If the C standard is not clear on this, it needs to be improved.
CVS commit: src/sys/compat/netbsd32
Module Name:src Committed By: rin Date: Thu Nov 7 15:21:56 UTC 2019 Modified Files: src/sys/compat/netbsd32: netbsd32.h Log Message: For netbsd32_statvfs, f_spare should be netbsd32_uint64, not uint64_t. Fix syscalls using struct statvfs on COMPAT_NETBSD32 on amd64, where NETBSD32_INT64_ALIGN is __attribute__((__aligned__(4))). To generate a diff of this commit: cvs rdiff -u -r1.127 -r1.128 src/sys/compat/netbsd32/netbsd32.h Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files.
CVS commit: src/sys/compat/netbsd32
Module Name:src
Committed By: rin
Date: Thu Nov 7 15:21:56 UTC 2019
Modified Files:
src/sys/compat/netbsd32: netbsd32.h
Log Message:
For netbsd32_statvfs, f_spare should be netbsd32_uint64, not uint64_t.
Fix syscalls using struct statvfs on COMPAT_NETBSD32 on amd64, where
NETBSD32_INT64_ALIGN is __attribute__((__aligned__(4))).
To generate a diff of this commit:
cvs rdiff -u -r1.127 -r1.128 src/sys/compat/netbsd32/netbsd32.h
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
Modified files:
Index: src/sys/compat/netbsd32/netbsd32.h
diff -u src/sys/compat/netbsd32/netbsd32.h:1.127 src/sys/compat/netbsd32/netbsd32.h:1.128
--- src/sys/compat/netbsd32/netbsd32.h:1.127 Thu Oct 3 22:16:53 2019
+++ src/sys/compat/netbsd32/netbsd32.h Thu Nov 7 15:21:55 2019
@@ -1,4 +1,4 @@
-/* $NetBSD: netbsd32.h,v 1.127 2019/10/03 22:16:53 kamil Exp $ */
+/* $NetBSD: netbsd32.h,v 1.128 2019/11/07 15:21:55 rin Exp $ */
/*
* Copyright (c) 1998, 2001, 2008, 2015 Matthew R. Green
@@ -902,7 +902,7 @@ struct netbsd32_statvfs {
netbsd32_u_long f_fsid; /* Posix compatible fsid */
netbsd32_u_long f_namemax; /* maximum filename length */
uid_t f_owner; /* user that mounted the file system */
- uint64_t f_spare[4]; /* spare space */
+ netbsd32_uint64 f_spare[4]; /* spare space */
char f_fstypename[_VFS_NAMELEN]; /* fs type name */
char f_mntonname[_VFS_MNAMELEN]; /* directory on which mounted */
char f_mntfromname[_VFS_MNAMELEN]; /* mounted file system */
Re: CVS commit: src/sys/kern
On 07.11.2019 14:25, Valery Ushakov wrote: > If the sanitizer does complain about other uses, there is little point > in fixing one instance and not the others. We already agreed with Christos that this is appeasing of GCC. If you want to scan the whole kernel (or whole C) file for more occurrences of violations - please go for it. signature.asc Description: OpenPGP digital signature
Re: CVS commit: src/sys/kern
On Thu, Nov 07, 2019 at 15:48:55 +0300, Valery Ushakov wrote: > On Thu, Nov 07, 2019 at 13:37:21 +0100, Kamil Rytarowski wrote: > > > On 07.11.2019 13:17, Valery Ushakov wrote: > > > On Thu, Nov 07, 2019 at 06:02:39 +0100, Kamil Rytarowski wrote: > > > > > >> I have checked received the following patch and received a feedback from > > >> a LLVM developer. > > >> > > >> On 07.11.2019 05:47, 'Dmitry Vyukov' via syzkaller-netbsd-bugs wrote: > > >>> I've consulted with some people and _presumably_ (to the degree one > > >>> can be sure about bitter corner cases of C/C++ :)) this is a correct > > >>> fix (and formally correct warnings from ubsan). > > >>> As 6.5.3.2/4 says, only &*p and [i] syntactic forms are defined as > > >>> special case of not being a dereference, but rather effectively an > > >>> address calculation. But >m is not. Thus it is interpreted as a > > >>> dereference that produces an lvalue and then taking address of that > > >>> lvalue. At the point of dereference we have UB. Your fix avoids the > > >>> dereference. > > > > > > The context is lost in the thread, but the original change was about > > > >d_magic as far as I can figure out. If the claim is that that's > > > UB b/c dlp is improperly aligned, then why the half of the rest of the > > > file is not UB as it uses the same "dlp" pointer to access other > > > members of the disklabel. > > > > We were already addressing various reports for disklabel related code in > > the kernel and userland. In userland we as far as I recall just copy the > > struct into an aligned promptly pointer. > > > > There might be more problems, but we address them as they pop up. > > That seems counterintuitive. There's the root cause and when/if that > root cause is fixed, then this particular problem will be fixed as > well. The concern obviously is that when the root problem is fixed, > this change will be forgotten and the unnecessarily uglified code will > be just left over as it currently is. > > So the change is extremely questionable from that point of view. It > creates the illusion of things being "fixed" while in the longer run > I'd say it harms the code. And as I said, if the sanitizer flags that > place as UB and doesn't flag dozens of dlp->foo accesses elsewhere in > that file, then either I don't understand what the sanitizer complains > about, or it's not a very good sanitizer and we shouldn't care to shut > it up in this random place that triggers it. Lest the legalese part of this sub-thread distract from this point let me re-iterate. I'm not questioning primarily whether that >d_magic is UB or not (it most likely is). What irks me about this change is that we tweak one random instance of a larger problem and disregard the rest of it. If I misunderstand the problem and other uses of dpl->foo in the same file are ok, please, kindly point this out to me. If the other uses are indeed problematic, then does or doesn't the sanitizer complain about them, like it complains about that check that was "fixed" in the original commit? If the sanitizer does complain about other uses, there is little point in fixing one instance and not the others. If the sanitizer does NOT complain about other uses, then please find a different way to shup the stupid thing up. (Which is also how I read the responses from Martin and Christos, but that's just my interpretation). -uwe
Re: CVS commit: src/sys/kern
On Thu, Nov 07, 2019 at 13:59:37 +0100, Kamil Rytarowski wrote: > On 07.11.2019 13:48, Valery Ushakov wrote: > > On Thu, Nov 07, 2019 at 13:37:21 +0100, Kamil Rytarowski wrote: > > > >> On 07.11.2019 13:17, Valery Ushakov wrote: > >>> On Thu, Nov 07, 2019 at 06:02:39 +0100, Kamil Rytarowski wrote: > >>> As a side note - the C99 standard contains "derefer" exactly once, in > >>> a footnote. Since we have ended up in the darkest corners of > >>> legalistic exegesis, please, can we avoid using the word that is, > >>> technically speaking, meaningless as far as this discussion is > >>> concerned? > >> > >> Unary * oprator. C++ specified term "dereferenceable" in the context of > >> the unary * operator. > > > > This is C code and the C standard is hard enough as it is already. > > Please, can we put the C++ aside for a moment? > > No. The kernel was already patched (years ago) to build as a valid C++ > software. "No" what? This is C code. If it also happens to be a valid C++ code, good for it, but that is a separate matter. There's a claim made about that code that it triggers UB according to the C standard. That claim can be meaningfully dicussed in that legal(ese) framework only. Only after the meaning of the competing claims about that code is clarified within its "native" framework can we consider C++ interpretation of the same code as a followup and whatever interplay there is between the standards. So, if that code is supposed to be valid C code *and* valid C++ code, then it should be at least valid C code and so, please, can we for now stick to that part of the "and"? -uwe
Re: CVS commit: src/sys/kern
On 07.11.2019 13:48, Valery Ushakov wrote: > On Thu, Nov 07, 2019 at 13:37:21 +0100, Kamil Rytarowski wrote: > >> On 07.11.2019 13:17, Valery Ushakov wrote: >>> On Thu, Nov 07, 2019 at 06:02:39 +0100, Kamil Rytarowski wrote: >>> As a side note - the C99 standard contains "derefer" exactly once, in >>> a footnote. Since we have ended up in the darkest corners of >>> legalistic exegesis, please, can we avoid using the word that is, >>> technically speaking, meaningless as far as this discussion is >>> concerned? >> >> Unary * oprator. C++ specified term "dereferenceable" in the context of >> the unary * operator. > > This is C code and the C standard is hard enough as it is already. > Please, can we put the C++ aside for a moment? > No. The kernel was already patched (years ago) to build as a valid C++ software. > > -uwe > signature.asc Description: OpenPGP digital signature
Re: CVS commit: src/sys/kern
On Thu, Nov 07, 2019 at 13:37:21 +0100, Kamil Rytarowski wrote: > On 07.11.2019 13:17, Valery Ushakov wrote: > > On Thu, Nov 07, 2019 at 06:02:39 +0100, Kamil Rytarowski wrote: > > > >> I have checked received the following patch and received a feedback from > >> a LLVM developer. > >> > >> On 07.11.2019 05:47, 'Dmitry Vyukov' via syzkaller-netbsd-bugs wrote: > >>> I've consulted with some people and _presumably_ (to the degree one > >>> can be sure about bitter corner cases of C/C++ :)) this is a correct > >>> fix (and formally correct warnings from ubsan). > >>> As 6.5.3.2/4 says, only &*p and [i] syntactic forms are defined as > >>> special case of not being a dereference, but rather effectively an > >>> address calculation. But >m is not. Thus it is interpreted as a > >>> dereference that produces an lvalue and then taking address of that > >>> lvalue. At the point of dereference we have UB. Your fix avoids the > >>> dereference. > > > > The context is lost in the thread, but the original change was about > > >d_magic as far as I can figure out. If the claim is that that's > > UB b/c dlp is improperly aligned, then why the half of the rest of the > > file is not UB as it uses the same "dlp" pointer to access other > > members of the disklabel. > > We were already addressing various reports for disklabel related code in > the kernel and userland. In userland we as far as I recall just copy the > struct into an aligned promptly pointer. > > There might be more problems, but we address them as they pop up. That seems counterintuitive. There's the root cause and when/if that root cause is fixed, then this particular problem will be fixed as well. The concern obviously is that when the root problem is fixed, this change will be forgotten and the unnecessarily uglified code will be just left over as it currently is. So the change is extremely questionable from that point of view. It creates the illusion of things being "fixed" while in the longer run I'd say it harms the code. And as I said, if the sanitizer flags that place as UB and doesn't flag dozens of dlp->foo accesses elsewhere in that file, then either I don't understand what the sanitizer complains about, or it's not a very good sanitizer and we shouldn't care to shut it up in this random place that triggers it. > > As a side note - the C99 standard contains "derefer" exactly once, in > > a footnote. Since we have ended up in the darkest corners of > > legalistic exegesis, please, can we avoid using the word that is, > > technically speaking, meaningless as far as this discussion is > > concerned? > > Unary * oprator. C++ specified term "dereferenceable" in the context of > the unary * operator. This is C code and the C standard is hard enough as it is already. Please, can we put the C++ aside for a moment? -uwe
Re: CVS commit: src/sys/kern
On 07.11.2019 13:17, Valery Ushakov wrote: > On Thu, Nov 07, 2019 at 06:02:39 +0100, Kamil Rytarowski wrote: > >> I have checked received the following patch and received a feedback from >> a LLVM developer. >> >> On 07.11.2019 05:47, 'Dmitry Vyukov' via syzkaller-netbsd-bugs wrote: >>> I've consulted with some people and _presumably_ (to the degree one >>> can be sure about bitter corner cases of C/C++ :)) this is a correct >>> fix (and formally correct warnings from ubsan). >>> As 6.5.3.2/4 says, only &*p and [i] syntactic forms are defined as >>> special case of not being a dereference, but rather effectively an >>> address calculation. But >m is not. Thus it is interpreted as a >>> dereference that produces an lvalue and then taking address of that >>> lvalue. At the point of dereference we have UB. Your fix avoids the >>> dereference. > > The context is lost in the thread, but the original change was about > >d_magic as far as I can figure out. If the claim is that that's > UB b/c dlp is improperly aligned, then why the half of the rest of the > file is not UB as it uses the same "dlp" pointer to access other > members of the disklabel. > We were already addressing various reports for disklabel related code in the kernel and userland. In userland we as far as I recall just copy the struct into an aligned promptly pointer. There might be more problems, but we address them as they pop up. > As a side note - the C99 standard contains "derefer" exactly once, in > a footnote. Since we have ended up in the darkest corners of > legalistic exegesis, please, can we avoid using the word that is, > technically speaking, meaningless as far as this discussion is > concerned? > > -uwe > Unary * oprator. C++ specified term "dereferenceable" in the context of the unary * operator. signature.asc Description: OpenPGP digital signature
Re: CVS commit: src/sys/kern
On Thu, Nov 07, 2019 at 06:02:39 +0100, Kamil Rytarowski wrote: > I have checked received the following patch and received a feedback from > a LLVM developer. > > On 07.11.2019 05:47, 'Dmitry Vyukov' via syzkaller-netbsd-bugs wrote: > > I've consulted with some people and _presumably_ (to the degree one > > can be sure about bitter corner cases of C/C++ :)) this is a correct > > fix (and formally correct warnings from ubsan). > > As 6.5.3.2/4 says, only &*p and [i] syntactic forms are defined as > > special case of not being a dereference, but rather effectively an > > address calculation. But >m is not. Thus it is interpreted as a > > dereference that produces an lvalue and then taking address of that > > lvalue. At the point of dereference we have UB. Your fix avoids the > > dereference. The context is lost in the thread, but the original change was about >d_magic as far as I can figure out. If the claim is that that's UB b/c dlp is improperly aligned, then why the half of the rest of the file is not UB as it uses the same "dlp" pointer to access other members of the disklabel. As a side note - the C99 standard contains "derefer" exactly once, in a footnote. Since we have ended up in the darkest corners of legalistic exegesis, please, can we avoid using the word that is, technically speaking, meaningless as far as this discussion is concerned? -uwe
Re: CVS commit: src/sys/kern
On 07.11.2019 11:53, Martin Husemann wrote: > On Thu, Nov 07, 2019 at 11:46:47AM +0100, Kamil Rytarowski wrote: >> Please see my newer mail with rationale and another one with a >> confirmation that this was real UB. > > Confirmation? The dereference in this case happens in memcmp() > only, so what misalignment could there be? > > Martin > It was (as I called it) syntactical dereference A->b, followed by retrieving of its address &(). C does not define this syntax as optimizable (removal of dereference) and there is UB (but not real life). There are different semantics that are defined, such as *&, but >b is not one of them (certainly it could - I can write a proposal to the C standard). signature.asc Description: OpenPGP digital signature
Re: CVS commit: src/sys/kern
On Thu, Nov 07, 2019 at 11:46:47AM +0100, Kamil Rytarowski wrote: > Please see my newer mail with rationale and another one with a > confirmation that this was real UB. Confirmation? The dereference in this case happens in memcmp() only, so what misalignment could there be? Martin
Re: CVS commit: src/sys/kern
On 07.11.2019 07:25, Martin Husemann wrote: > On Wed, Nov 06, 2019 at 11:17:23PM +0100, Kamil Rytarowski wrote: >> Technically, I think that this is a real UB. >> >> 6.3.2.3/7 >> A pointer to an object type may be converted to a pointer to a >> different object type. If the resulting pointer is not correctly >> aligned for the referenced type, the behavior is undefined. > > I don't think this applies here, as the usage was invoking memcmp(), > so explicitly being fine with unaligned data. > > This clearly was a false positive and the code is now a lot more > unreadable. > > Martin > Please see my newer mail with rationale and another one with a confirmation that this was real UB. signature.asc Description: OpenPGP digital signature
CVS commit: src/sys/dev/pci
Module Name:src Committed By: nisimura Date: Thu Nov 7 09:05:29 UTC 2019 Modified Files: src/sys/dev/pci: if_kse.c Log Message: clarify 8842 MAC behaves 100FDX only has no alternative media selection possible. To generate a diff of this commit: cvs rdiff -u -r1.39 -r1.40 src/sys/dev/pci/if_kse.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files.
CVS commit: src/sys/dev/pci
Module Name:src
Committed By: nisimura
Date: Thu Nov 7 09:05:29 UTC 2019
Modified Files:
src/sys/dev/pci: if_kse.c
Log Message:
clarify 8842 MAC behaves 100FDX only has no alternative media selection
possible.
To generate a diff of this commit:
cvs rdiff -u -r1.39 -r1.40 src/sys/dev/pci/if_kse.c
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
Modified files:
Index: src/sys/dev/pci/if_kse.c
diff -u src/sys/dev/pci/if_kse.c:1.39 src/sys/dev/pci/if_kse.c:1.40
--- src/sys/dev/pci/if_kse.c:1.39 Wed Nov 6 14:33:52 2019
+++ src/sys/dev/pci/if_kse.c Thu Nov 7 09:05:29 2019
@@ -1,4 +1,4 @@
-/* $NetBSD: if_kse.c,v 1.39 2019/11/06 14:33:52 nisimura Exp $ */
+/* $NetBSD: if_kse.c,v 1.40 2019/11/07 09:05:29 nisimura Exp $ */
/*-
* Copyright (c) 2006 The NetBSD Foundation, Inc.
@@ -30,8 +30,7 @@
*/
#include
-__KERNEL_RCSID(0, "$NetBSD: if_kse.c,v 1.39 2019/11/06 14:33:52 nisimura Exp $");
-
+__KERNEL_RCSID(0, "$NetBSD: if_kse.c,v 1.40 2019/11/07 09:05:29 nisimura Exp $");
#include
#include
@@ -328,6 +327,7 @@ static void txreap(struct kse_softc *);
static void lnkchg(struct kse_softc *);
static int ksephy_change(struct ifnet *);
static void ksephy_status(struct ifnet *, struct ifmediareq *);
+static void nopifm_status(struct ifnet *, struct ifmediareq *);
static void phy_tick(void *);
#ifdef KSE_EVENT_COUNTERS
static void stat_tick(void *);
@@ -514,8 +514,16 @@ kse_attach(device_t parent, device_t sel
ifmedia_add(ifm, IFM_ETHER | IFM_AUTO, 0, NULL);
ifmedia_set(ifm, IFM_ETHER | IFM_AUTO);
} else {
- ifmedia_init(ifm, 0, NULL, NULL);
- ifmedia_add(ifm, IFM_ETHER | IFM_100_TX, 0, NULL);
+ /*
+ * pretend 100FDX w/ no alternative media selection.
+ * 8842 MAC is tied with a builtin 3 port switch.
+ * It can do rate control over either of tx / rx direction
+ * respectively, tough, this driver leaves the rate unlimited
+ * intending 100Mbps maxinum.
+ * 2 ports behave in AN mode and this driver provides no mean
+ * to see the exact details.
+ */
+ ifmedia_init(ifm, 0, NULL, nopifm_status);
ifmedia_add(ifm, IFM_ETHER | IFM_100_TX | IFM_FDX, 0, NULL);
ifmedia_set(ifm, IFM_ETHER | IFM_100_TX | IFM_FDX);
}
@@ -1345,6 +1353,21 @@ printf("P1SR: %04x link %s\n", p1sr, (p1
}
static void
+nopifm_status(struct ifnet *ifp, struct ifmediareq *ifmr)
+{
+ struct kse_softc *sc = ifp->if_softc;
+ struct ifmedia *ifm = >sc_media;
+
+#if KSE_LINKDEBUG > 1
+printf("p1sr: %04x, p2sr: %04x\n", CSR_READ_2(sc, P1SR), CSR_READ_2(sc, P2SR));
+#endif
+
+ /* 8842 MAC pretends 100FDX all the time */
+ ifmr->ifm_active = ifm->ifm_cur->ifm_media;
+ ifmr->ifm_status = IFM_AVALID | IFM_ACTIVE;
+}
+
+static void
phy_tick(void *arg)
{
struct kse_softc *sc = arg;
