Re: unlock mmap(2) for anonymous mappings
On Mon, Jan 24, 2022 at 12:08:33PM -0300, Martin Pieuchot wrote:
> On 24/01/22(Mon) 12:06, Klemens Nanni wrote:
> > On Sun, Jan 16, 2022 at 09:22:50AM -0300, Martin Pieuchot wrote:
> > > IMHO this approach of let's try if it works now and revert if it isn't
> > > doesn't help us make progress. I'd be more confident seeing diffs that
> > > assert for the right lock in the functions called by uvm_mapanon() and
> > > documentation about which lock is protecting which field of the data
> > > structures.
> >
> > I picked `vm_map's `size' as first underdocumented member.
> > All accesses to it are protected by vm_map_lock(), either through the
> > function itself or implicitly by already requiring the calling function
> > to lock the map.
>
> Could we use a vm_map_assert_locked() or something similar that reflect
> the exclusive or shared (read) lock comments? I don't trust comments.
> It's too easy to miss a lock in a code path.
This survives desktop usage, running regress and building kernels on
amd64.
I'll throw it at sparc64 soon.
>
> > So annotate functions using `size' wrt. the required lock.
Index: uvm_map.c
===
RCS file: /cvs/src/sys/uvm/uvm_map.c,v
retrieving revision 1.282
diff -u -p -r1.282 uvm_map.c
--- uvm_map.c 21 Dec 2021 22:21:32 - 1.282
+++ uvm_map.c 25 Jan 2022 07:37:32 -
@@ -805,6 +805,8 @@ uvm_map_sel_limits(vaddr_t *min, vaddr_t
* Fills in *start_ptr and *end_ptr to be the first and last entry describing
* the space.
* If called with prefilled *start_ptr and *end_ptr, they are to be correct.
+ *
+ * map must be at least read-locked.
*/
int
uvm_map_isavail(struct vm_map *map, struct uvm_addr_state *uaddr,
@@ -815,6 +817,8 @@ uvm_map_isavail(struct vm_map *map, stru
struct uvm_map_addr *atree;
struct vm_map_entry *i, *i_end;
+ vm_map_assert_locked(map);
+
if (addr + sz < addr)
return 0;
@@ -986,6 +990,8 @@ uvm_mapanon(struct vm_map *map, vaddr_t
vaddr_t pmap_align, pmap_offset;
vaddr_t hint;
+ vm_map_assert_unlocked(map);
+
KASSERT((map->flags & VM_MAP_ISVMSPACE) == VM_MAP_ISVMSPACE);
KASSERT(map != kernel_map);
KASSERT((map->flags & UVM_FLAG_HOLE) == 0);
@@ -1189,6 +1195,8 @@ uvm_map(struct vm_map *map, vaddr_t *add
vaddr_t pmap_align, pmap_offset;
vaddr_t hint;
+ vm_map_assert_unlocked(map);
+
if ((map->flags & VM_MAP_INTRSAFE) == 0)
splassert(IPL_NONE);
else
@@ -1821,6 +1829,8 @@ boolean_t
uvm_map_lookup_entry(struct vm_map *map, vaddr_t address,
struct vm_map_entry **entry)
{
+ vm_map_assert_locked(map);
+
*entry = uvm_map_entrybyaddr(&map->addr, address);
return *entry != NULL && !UVM_ET_ISHOLE(*entry) &&
(*entry)->start <= address && (*entry)->end > address;
@@ -2206,6 +2216,8 @@ uvm_unmap_kill_entry(struct vm_map *map,
* If remove_holes, then remove ET_HOLE entries as well.
* If markfree, entry will be properly marked free, otherwise, no replacement
* entry will be put in the tree (corrupting the tree).
+ *
+ * map must be locked.
*/
void
uvm_unmap_remove(struct vm_map *map, vaddr_t start, vaddr_t end,
@@ -2214,6 +2226,8 @@ uvm_unmap_remove(struct vm_map *map, vad
{
struct vm_map_entry *prev_hint, *next, *entry;
+ vm_map_assert_locked(map);
+
start = MAX(start, map->min_offset);
end = MIN(end, map->max_offset);
if (start >= end)
@@ -2976,13 +2990,17 @@ uvm_tree_sanity(struct vm_map *map, char
UVM_ASSERT(map, addr == vm_map_max(map), file, line);
}
+/*
+ * map must be at least read-locked.
+ */
void
uvm_tree_size_chk(struct vm_map *map, char *file, int line)
{
struct vm_map_entry *iter;
- vsize_t size;
+ vsize_t size = 0;
+
+ vm_map_assert_locked(map);
- size = 0;
RBT_FOREACH(iter, uvm_map_addr, &map->addr) {
if (!UVM_ET_ISHOLE(iter))
size += iter->end - iter->start;
@@ -3320,6 +3338,8 @@ uvm_map_protect(struct vm_map *map, vadd
vsize_t dused;
int error;
+ vm_map_assert_unlocked(map);
+
if (start > end)
return EINVAL;
start = MAX(start, map->min_offset);
@@ -4096,6 +4116,7 @@ uvmspace_fork(struct process *pr)
struct vm_map_entry *old_entry, *new_entry;
struct uvm_map_deadq dead;
+ vm_map_assert_unlocked(old_map);
vm_map_lock(old_map);
vm2 = uvmspace_alloc(old_map->min_offset, old_map->max_offset,
@@ -4236,6 +4257,8 @@ uvm_map_submap(struct vm_map *map, vaddr
struct vm_map_entry *entry;
int result;
+ vm_map_assert_unlocked(map);
+
if (start > map->max_offset || end > map->max_offset ||
start < map->min_offset || end < map->min_offset)
Re: Properly check if ACPI devices are enabled
On Mon, Jan 24, 2022 at 08:40:36PM +0100, Mark Kettenis wrote: > > Date: Mon, 24 Jan 2022 20:19:46 +0100 > > From: Anton Lindqvist > > > > On Mon, Jan 24, 2022 at 05:31:49PM +0100, Mark Kettenis wrote: > > > Currently we attach ACPI devices that are present in a machine. > > > However, in some cases ACPI devices can be present, but not enabled. > > > Attaching a device driver to devices that are not enabled is not a > > > good idea since reading and writing from/to its registers will fail > > > and the driver will malfunction in interesting ways. Such as a com(4) > > > serial port that is misdetected and hangs the kernel when it is > > > actually opened. > > > > > > The diff below makes sure we only enable devices that are actually > > > enabled. This may cause some devices to disappear in OpenBSD. > > > However those devices should have been unusable anyway, so that isn't > > > an issue. > > > > > > ok? > > > > According to the ACPI specification[1]: > > > > > A device can only decode its hardware resources if both bits 0 and 1 are > > > set. If the device is not present (bit [0] cleared) or not enabled (bit > > > [1] cleared), then the device must not decode its resources. > > Just before that it says: > > If bit [0] is cleared, then bit 1 must also be cleared (in other > words, a device that is not present cannot be enabled). > > > Should we therefore check for presence of both STA_PRESENT and > > STA_ENABLED? > > So according to the ACPI specification we don't need to do that. > Should we do it just to be safe? I would still vote for it.
Re: Capture a repeated pattern into sysctl_securelevel_int
On Sun, Jan 23, 2022 at 10:29:22PM -0800, Greg Steuck wrote:
> As I was staring a bit more at sysctl related code this pattern caught
> my attention. Looks like a few lines can disappear and hopefully code
> expressivity goes up.
>
> Anybody like this?
OK bluhm@
> >From 3b52f9ad743fe9b59316077478d33f77fff9a119 Mon Sep 17 00:00:00 2001
> From: Greg Steuck
> Date: Sun, 23 Jan 2022 22:05:45 -0800
> Subject: [PATCH] Capture a repeated pattern into sysctl_securelevel_int
> function
>
> A few variables in the kernel are only writeable before securelevel is
> raised. It makes sense to handle them with less code.
> ---
> sys/arch/amd64/amd64/machdep.c | 8 ++--
> sys/arch/i386/i386/machdep.c | 8 ++--
> sys/kern/kern_sysctl.c | 33 +++--
> sys/kern/vfs_subr.c| 5 ++---
> sys/netinet/ip_input.c | 7 +--
> sys/sys/sysctl.h | 1 +
> 6 files changed, 27 insertions(+), 35 deletions(-)
>
> diff --git a/sys/arch/amd64/amd64/machdep.c b/sys/arch/amd64/amd64/machdep.c
> index 326f1c10253..4f9585510a1 100644
> --- a/sys/arch/amd64/amd64/machdep.c
> +++ b/sys/arch/amd64/amd64/machdep.c
> @@ -513,12 +513,8 @@ cpu_sysctl(int *name, u_int namelen, void *oldp, size_t
> *oldlenp, void *newp,
> case CPU_CPUVENDOR:
> return (sysctl_rdstring(oldp, oldlenp, newp, cpu_vendor));
> case CPU_KBDRESET:
> - if (securelevel > 0)
> - return (sysctl_rdint(oldp, oldlenp, newp,
> - kbd_reset));
> - else
> - return (sysctl_int(oldp, oldlenp, newp, newlen,
> - &kbd_reset));
> + return (sysctl_securelevel_int(oldp, oldlenp, newp, newlen,
> + &kbd_reset));
> case CPU_ALLOWAPERTURE:
> if (namelen != 1)
> return (ENOTDIR); /* overloaded */
> diff --git a/sys/arch/i386/i386/machdep.c b/sys/arch/i386/i386/machdep.c
> index 0d32cd4edd7..56e30fac9df 100644
> --- a/sys/arch/i386/i386/machdep.c
> +++ b/sys/arch/i386/i386/machdep.c
> @@ -3617,12 +3617,8 @@ cpu_sysctl(int *name, u_int namelen, void *oldp,
> size_t *oldlenp, void *newp,
> case CPU_CPUFEATURE:
> return (sysctl_rdint(oldp, oldlenp, newp,
> curcpu()->ci_feature_flags));
> case CPU_KBDRESET:
> - if (securelevel > 0)
> - return (sysctl_rdint(oldp, oldlenp, newp,
> - kbd_reset));
> - else
> - return (sysctl_int(oldp, oldlenp, newp, newlen,
> - &kbd_reset));
> + return (sysctl_securelevel_int(oldp, oldlenp, newp, newlen,
> + &kbd_reset));
> #if NPCKBC > 0 && NUKBD > 0
> case CPU_FORCEUKBD:
> {
> diff --git a/sys/kern/kern_sysctl.c b/sys/kern/kern_sysctl.c
> index e71cdf5897d..fe6a2689365 100644
> --- a/sys/kern/kern_sysctl.c
> +++ b/sys/kern/kern_sysctl.c
> @@ -473,14 +473,12 @@ kern_sysctl(int *name, u_int namelen, void *oldp,
> size_t *oldlenp, void *newp,
> return (0);
> #if NDT > 0
> case KERN_ALLOWDT:
> - if (securelevel > 0)
> - return (sysctl_rdint(oldp, oldlenp, newp, allowdt));
> - return (sysctl_int(oldp, oldlenp, newp, newlen, &allowdt));
> + return (sysctl_securelevel_int(oldp, oldlenp, newp, newlen,
> + &allowdt));
> #endif
> case KERN_ALLOWKMEM:
> - if (securelevel > 0)
> - return (sysctl_rdint(oldp, oldlenp, newp, allowkmem));
> - return (sysctl_int(oldp, oldlenp, newp, newlen, &allowkmem));
> + return (sysctl_securelevel_int(oldp, oldlenp, newp, newlen,
> + &allowkmem));
> case KERN_HOSTNAME:
> error = sysctl_tstring(oldp, oldlenp, newp, newlen,
> hostname, sizeof(hostname));
> @@ -757,10 +755,7 @@ hw_sysctl(int *name, u_int namelen, void *oldp, size_t
> *oldlenp, void *newp,
> return (sysctl_rdquad(oldp, oldlenp, newp,
> ptoa((psize_t)physmem - uvmexp.wired)));
> case HW_ALLOWPOWERDOWN:
> - if (securelevel > 0)
> - return (sysctl_rdint(oldp, oldlenp, newp,
> - allowpowerdown));
> - return (sysctl_int(oldp, oldlenp, newp, newlen,
> + return (sysctl_securelevel_int(oldp, oldlenp, newp, newlen,
> &allowpowerdown));
> #ifdef __HAVE_CPU_TOPOLOGY
> case HW_SMT:
> @@ -875,6 +870,18 @@ sysctl_rdint(void *oldp, size_t *oldlenp, void *newp,
> int val)
> return (error);
> }
>
> +/*
> + * Selects between sysctl_rdint and sysctl_int according to securelevel.
> + */
> +int
> +sysctl_securelevel_int(void *oldp, size_t *oldlenp, void *newp, size_t
> newlen,
> +int *valp)
> +{
> + if (securelevel > 0)
> + return (sysctl_
kubsan em ix ixgbe
Hi
There are more undefined behaviour reports in our network drivers
kubsan: dev/pci/if_em_hw.c:7625:38: shift: left shift of 65535 by 16 places
cannot be represented in type 'int'
kubsan: dev/pci/if_ix.c:3403:18: shift: left shift of 255 by 24 places cannot
be represented in type 'int'
kubsan: dev/pci/if_ix.c:3404:19: shift: left shift of 131 by 24 places cannot
be represented in type 'int'
kubsan: dev/pci/ixgbe_82598.c:547:26: signed integer overflow: 65535 * 65537
cannot be represented in type 'int'
kubsan: dev/pci/if_ix.c:3422:19: shift: left shift of 255 by 24 places cannot
be represented in type 'int'
kubsan: dev/pci/if_ix.c:3423:20: shift: left shift of 129 by 24 places cannot
be represented in type 'int'
kubsan: dev/pci/ixgbe.c:2381:26: signed integer overflow: 65535 * 65537 cannot
be represented in type 'int'
ok?
bluhm
Index: dev/pci/if_em_hw.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/dev/pci/if_em_hw.c,v
retrieving revision 1.113
diff -u -p -r1.113 if_em_hw.c
--- dev/pci/if_em_hw.c 9 Jan 2022 05:42:50 - 1.113
+++ dev/pci/if_em_hw.c 24 Jan 2022 17:23:25 -
@@ -7622,7 +7622,7 @@ em_read_part_num(struct em_hw *hw, uint3
return -E1000_ERR_EEPROM;
}
/* Save word 0 in upper half of part_num */
- *part_num = (uint32_t) (eeprom_data << 16);
+ *part_num = (uint32_t)eeprom_data << 16;
/* Get word 1 from EEPROM */
if (em_read_eeprom(hw, ++offset, 1, &eeprom_data) < 0) {
Index: dev/pci/if_ix.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/dev/pci/if_ix.c,v
retrieving revision 1.180
diff -u -p -r1.180 if_ix.c
--- dev/pci/if_ix.c 27 Jul 2021 01:44:55 - 1.180
+++ dev/pci/if_ix.c 24 Jan 2022 17:23:25 -
@@ -3400,8 +3400,8 @@ ixgbe_set_ivar(struct ix_softc *sc, uint
entry += (type * 64);
index = (entry >> 2) & 0x1F;
ivar = IXGBE_READ_REG(hw, IXGBE_IVAR(index));
- ivar &= ~(0xFF << (8 * (entry & 0x3)));
- ivar |= (vector << (8 * (entry & 0x3)));
+ ivar &= ~((uint32_t)0xFF << (8 * (entry & 0x3)));
+ ivar |= ((uint32_t)vector << (8 * (entry & 0x3)));
IXGBE_WRITE_REG(&sc->hw, IXGBE_IVAR(index), ivar);
break;
@@ -3413,14 +3413,14 @@ ixgbe_set_ivar(struct ix_softc *sc, uint
if (type == -1) { /* MISC IVAR */
index = (entry & 1) * 8;
ivar = IXGBE_READ_REG(hw, IXGBE_IVAR_MISC);
- ivar &= ~(0xFF << index);
- ivar |= (vector << index);
+ ivar &= ~((uint32_t)0xFF << index);
+ ivar |= ((uint32_t)vector << index);
IXGBE_WRITE_REG(hw, IXGBE_IVAR_MISC, ivar);
} else {/* RX/TX IVARS */
index = (16 * (entry & 1)) + (8 * type);
ivar = IXGBE_READ_REG(hw, IXGBE_IVAR(entry >> 1));
- ivar &= ~(0xFF << index);
- ivar |= (vector << index);
+ ivar &= ~((uint32_t)0xFF << index);
+ ivar |= ((uint32_t)vector << index);
IXGBE_WRITE_REG(hw, IXGBE_IVAR(entry >> 1), ivar);
}
Index: dev/pci/ixgbe.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/dev/pci/ixgbe.c,v
retrieving revision 1.26
diff -u -p -r1.26 ixgbe.c
--- dev/pci/ixgbe.c 2 Mar 2020 01:59:01 - 1.26
+++ dev/pci/ixgbe.c 24 Jan 2022 17:23:25 -
@@ -2378,7 +2378,7 @@ int32_t ixgbe_fc_enable_generic(struct i
}
/* Configure pause time (2 TCs per register) */
- reg = hw->fc.pause_time * 0x00010001;
+ reg = (uint32_t)hw->fc.pause_time * 0x00010001;
for (i = 0; i < (IXGBE_DCB_MAX_TRAFFIC_CLASS / 2); i++)
IXGBE_WRITE_REG(hw, IXGBE_FCTTV(i), reg);
Index: dev/pci/ixgbe_82598.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/dev/pci/ixgbe_82598.c,v
retrieving revision 1.19
diff -u -p -r1.19 ixgbe_82598.c
--- dev/pci/ixgbe_82598.c 9 Jan 2022 05:42:56 - 1.19
+++ dev/pci/ixgbe_82598.c 24 Jan 2022 17:23:25 -
@@ -544,7 +544,7 @@ int32_t ixgbe_fc_enable_82598(struct ixg
}
/* Configure pause time (2 TCs per register) */
- reg = hw->fc.pause_time * 0x00010001;
+ reg = (uint32_t)hw->fc.pause_time * 0x00010001;
for (i = 0; i < (IXGBE_DCB_MAX_TRAFFIC_CLASS / 2); i++)
IXGBE_WRITE_REG(hw, IXGBE_FCTTV(i), reg);
Re: Properly check if ACPI devices are enabled
On Mon, Jan 24, 2022 at 11:41 AM Mark Kettenis wrote: > > Date: Mon, 24 Jan 2022 20:19:46 +0100 > > From: Anton Lindqvist > > > > On Mon, Jan 24, 2022 at 05:31:49PM +0100, Mark Kettenis wrote: > > > Currently we attach ACPI devices that are present in a machine. > > > However, in some cases ACPI devices can be present, but not enabled. > > > Attaching a device driver to devices that are not enabled is not a > > > good idea since reading and writing from/to its registers will fail > > > and the driver will malfunction in interesting ways. Such as a com(4) > > > serial port that is misdetected and hangs the kernel when it is > > > actually opened. > > > > > > The diff below makes sure we only enable devices that are actually > > > enabled. This may cause some devices to disappear in OpenBSD. > > > However those devices should have been unusable anyway, so that isn't > > > an issue. > > > > > > ok? > > > > According to the ACPI specification[1]: > > > > > A device can only decode its hardware resources if both bits 0 and 1 > are > > > set. If the device is not present (bit [0] cleared) or not enabled (bit > > > [1] cleared), then the device must not decode its resources. > > Just before that it says: > > If bit [0] is cleared, then bit 1 must also be cleared (in other > words, a device that is not present cannot be enabled). > > > Should we therefore check for presence of both STA_PRESENT and > > STA_ENABLED? > > So according to the ACPI specification we don't need to do that. > Should we do it just to be safe? > Unless you're taking money bets about this being the one thing in the ACPI spec that some vendor won't screw up, doing both seems "can't be worse; can be better". Philip
Re: Properly check if ACPI devices are enabled
> Date: Mon, 24 Jan 2022 20:19:46 +0100 > From: Anton Lindqvist > > On Mon, Jan 24, 2022 at 05:31:49PM +0100, Mark Kettenis wrote: > > Currently we attach ACPI devices that are present in a machine. > > However, in some cases ACPI devices can be present, but not enabled. > > Attaching a device driver to devices that are not enabled is not a > > good idea since reading and writing from/to its registers will fail > > and the driver will malfunction in interesting ways. Such as a com(4) > > serial port that is misdetected and hangs the kernel when it is > > actually opened. > > > > The diff below makes sure we only enable devices that are actually > > enabled. This may cause some devices to disappear in OpenBSD. > > However those devices should have been unusable anyway, so that isn't > > an issue. > > > > ok? > > According to the ACPI specification[1]: > > > A device can only decode its hardware resources if both bits 0 and 1 are > > set. If the device is not present (bit [0] cleared) or not enabled (bit > > [1] cleared), then the device must not decode its resources. Just before that it says: If bit [0] is cleared, then bit 1 must also be cleared (in other words, a device that is not present cannot be enabled). > Should we therefore check for presence of both STA_PRESENT and > STA_ENABLED? So according to the ACPI specification we don't need to do that. Should we do it just to be safe? > [1] > https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/06_Device_Configuration/Device_Configuration.html#device-insertion-removal-and-status-objects
Re: Properly check if ACPI devices are enabled
On Mon, Jan 24, 2022 at 05:31:49PM +0100, Mark Kettenis wrote: > Currently we attach ACPI devices that are present in a machine. > However, in some cases ACPI devices can be present, but not enabled. > Attaching a device driver to devices that are not enabled is not a > good idea since reading and writing from/to its registers will fail > and the driver will malfunction in interesting ways. Such as a com(4) > serial port that is misdetected and hangs the kernel when it is > actually opened. > > The diff below makes sure we only enable devices that are actually > enabled. This may cause some devices to disappear in OpenBSD. > However those devices should have been unusable anyway, so that isn't > an issue. > > ok? According to the ACPI specification[1]: > A device can only decode its hardware resources if both bits 0 and 1 are > set. If the device is not present (bit [0] cleared) or not enabled (bit > [1] cleared), then the device must not decode its resources. Should we therefore check for presence of both STA_PRESENT and STA_ENABLED? [1] https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/06_Device_Configuration/Device_Configuration.html#device-insertion-removal-and-status-objects
Re: IPv6 privacy extensions
On 2022-01-24 00:17 +01, Marcel Logen <33327110-0...@ybtra.de> wrote: > Hello, > > since ca. April 2021 I see, that (after boot) no new IPv6 > temporary adresses are created after 900 seconds (15 min). > > The pltime decreases to 900 and then gets a value of 1800. > No new temporary address is generated. > > Is this behaviour correct? Yes. slaacd(8) used to never renew temporary addresses but would form a new one once the pltime reached 0. This was because I misunderstood what the RFC was saying and did not appreciate what would hapen. If there is a huge difference between pltime and vltime in router advertisements slaacd would accumulate deprecated temporary IPv6 addresses and wouldn't be able to get rid of them since the vltime did not expire. Say you have a pltime of 5 minutes and a vltime of a day. You'd get a new temporary address every 5 minutes and they would stick around for a day. You'd end up with 288 temporary IPv6 addresses. Long story short, this was fixed in rev 1.57 in engine.c See RFC 8981 3.4. step 1. -- I'm not entirely sure you are real.
Re: rpki-client MFT file and hash check change
On Mon, Jan 24, 2022 at 06:50:51PM +0100, Claudio Jeker wrote:
> > Requiring any content to be present (and the file's calculated digest to
> > match with the hash listed on the Manifest) might pose a problem with
> > our --exclude/--include rsync filter: files of unknown type are not
> > downloaded.
>
> Yes, this is an open issue that I have no great solution right now.
> Another issue is that these files are not moved to valid/ so a run
> with -n will fail on the MFT because the file is not present. I try to
> figure out a solution for these issues. Maybe it is enough to just
> ignore invalid non-existing files in proc_parser_mft_check().
I think the decision tree should be made as following:
1/ If a manifest contains a filename that is invalid according to
draft-ietf-sidrops-6486bis, reject the entire manifest. For example if
a filename contains two periods ('..'): Death sentence for that
CARepository.
2/ If a manifest contains a valid filename, which however is of a type
that rpki-client does not recognize: just 'skip over' that specific
entry. Unsupported filetypes can be recognized through their extension.
The referenced file is unlikely to be present in the incoming rsync
directory anyway; because of the --exclude/--include filters.
Forward compatibility is the objective.
Kind regards,
Job
Re: rpki-client MFT file and hash check change
On Mon, Jan 24, 2022 at 05:20:49PM +, Job Snijders wrote: > On Mon, Jan 24, 2022 at 04:33:10PM +0100, Claudio Jeker wrote: > > This diff does a few things regarding MFT file and hash sequences: > > > > - it validates the filename early on so that if considered valid it can > > be printed by printf(%s) without problems. > > - it assigns the file type (based on the file extension) early on and no > > longer uses this information when comparing the file hash. > > - Handle unknown files more like a soft error, the file hash still needs > > to match but the content is totally ignored. > > > > In other words it no longer rejects MFTs with unknown files in it. > > Right now rpki-client is very strict in what is accepted and it will > > become an some issue when ASPA is becomming more concrete. > > The valid_filename() function (which was re-factored away last week) was > intended to skip over FileAndHash entries which are not recognized by > that version of the rpki-client instance; precisely to support future > applications of the RPKI such as ASPA. > > https://github.com/openbsd/src/commit/dbcbf675726d8774c0bfc9925bda9c36edcb8a93 It did not really do that because any unknown file was considered invalid and with that the MFT failed to parse. > Requiring any content to be present (and the file's calculated digest to > match with the hash listed on the Manifest) might pose a problem with > our --exclude/--include rsync filter: files of unknown type are not > downloaded. Yes, this is an open issue that I have no great solution right now. Another issue is that these files are not moved to valid/ so a run with -n will fail on the MFT because the file is not present. I try to figure out a solution for these issues. Maybe it is enough to just ignore invalid non-existing files in proc_parser_mft_check(). > I think it would be best to check whether the filename on the manifest > conforms to draft-ietf-sidrops-6486bis, but if it is of an unknown type > to ignore it and not attempt to calculate/compare the hash. The filename still must conform to draft-ietf-sidrops-6486bis. Non-conforming filenames are a hard parse error for the MFT. > Perhaps it is worth expanding the return types from rtype_from_mftfile() > to differentiate between INVALID and UNKNOWN to make this easier? I think we could just rename INVALID to UNKNOWN but I need to double check with the source. I doubt we want both unless there is a really good reason for it. -- :wq Claudio
Re: rpki-client MFT file and hash check change
On Mon, Jan 24, 2022 at 04:33:10PM +0100, Claudio Jeker wrote: > This diff does a few things regarding MFT file and hash sequences: > > - it validates the filename early on so that if considered valid it can > be printed by printf(%s) without problems. > - it assigns the file type (based on the file extension) early on and no > longer uses this information when comparing the file hash. > - Handle unknown files more like a soft error, the file hash still needs > to match but the content is totally ignored. > > In other words it no longer rejects MFTs with unknown files in it. > Right now rpki-client is very strict in what is accepted and it will > become an some issue when ASPA is becomming more concrete. The valid_filename() function (which was re-factored away last week) was intended to skip over FileAndHash entries which are not recognized by that version of the rpki-client instance; precisely to support future applications of the RPKI such as ASPA. https://github.com/openbsd/src/commit/dbcbf675726d8774c0bfc9925bda9c36edcb8a93 Requiring any content to be present (and the file's calculated digest to match with the hash listed on the Manifest) might pose a problem with our --exclude/--include rsync filter: files of unknown type are not downloaded. I think it would be best to check whether the filename on the manifest conforms to draft-ietf-sidrops-6486bis, but if it is of an unknown type to ignore it and not attempt to calculate/compare the hash. Perhaps it is worth expanding the return types from rtype_from_mftfile() to differentiate between INVALID and UNKNOWN to make this easier? Kind regards, Job
Properly check if ACPI devices are enabled
Currently we attach ACPI devices that are present in a machine. However, in some cases ACPI devices can be present, but not enabled. Attaching a device driver to devices that are not enabled is not a good idea since reading and writing from/to its registers will fail and the driver will malfunction in interesting ways. Such as a com(4) serial port that is misdetected and hangs the kernel when it is actually opened. The diff below makes sure we only enable devices that are actually enabled. This may cause some devices to disappear in OpenBSD. However those devices should have been unusable anyway, so that isn't an issue. ok? Index: dev/acpi/acpi.c === RCS file: /cvs/src/sys/dev/acpi/acpi.c,v retrieving revision 1.405 diff -u -p -r1.405 acpi.c --- dev/acpi/acpi.c 12 Jan 2022 11:18:30 - 1.405 +++ dev/acpi/acpi.c 23 Jan 2022 17:13:03 - @@ -3321,7 +3321,7 @@ acpi_foundhid(struct aml_node *node, voi return (0); sta = acpi_getsta(sc, node->parent); - if ((sta & STA_PRESENT) == 0) + if ((sta & STA_ENABLED) == 0) return (0); if (aml_evalinteger(sc, node->parent, "_CCA", 0, NULL, &cca))
Re: rpki-client MFT file and hash check change
On Mon, Jan 24, 2022 at 04:33:10PM +0100, Claudio Jeker wrote:
> This diff does a few things regarding MFT file and hash sequences:
>
> - it validates the filename early on so that if considered valid it can
> be printed by printf(%s) without problems.
> - it assigns the file type (based on the file extension) early on and no
> longer uses this information when comparing the file hash.
> - Handle unknown files more like a soft error, the file hash still needs
> to match but the content is totally ignored.
>
> In other words it no longer rejects MFTs with unknown files in it.
> Right now rpki-client is very strict in what is accepted and it will
> become an some issue when ASPA is becomming more concrete.
Makes sense to me.
ok
One small suggestion below.
> Index: mft.c
> ===
[...]
> }
> + if (!valid_filename(file->value.ia5string->data,
> + file->value.ia5string->length)) {
> + warnx("%s: RFC 6486 section 4.2.2: bad filename", p->fn);
> + goto out;
> + }
> fn = strndup((const char *)file->value.ia5string->data,
> file->value.ia5string->length);
> if (fn == NULL)
> err(1, NULL);
>
> - if ((type = rtype_from_mftfile(fn)) == RTYPE_INVALID) {
> - warnx("%s: invalid filename: %s", p->fn, fn);
> - goto out;
> - }
> + type = rtype_from_mftfile(fn);
Now that we no longer check the return value, I would remove 'type'
from mft_parse_filehash() and assign the rtype directly to fent->type
a few lines down:
fent->file = fn;
fent->type = rtype_from_mftfile(fn);
fn = NULL;
>
> /* Now hash value. */
>
Re: syzcaller pf unhandled af
Hello,
I'm fine with the fix. I have just small comment/nit. leaving it
up to you to fix it ore leave it.
> -
> if (newrule->rt && !newrule->direction) {
> pf_rule_free(newrule);
> error = EINVAL;
> @@ -3216,6 +3194,34 @@ pf_rule_copyin(struct pf_rule *from, str
> to->set_prio[1] = from->set_prio[1];
>
> return (0);
> +}
> +
> +int
> +pf_rule_checkaf(struct pf_rule *r)
> +{
> + switch (r->af) {
> + case 0:
> + if (r->rule_flag & PFRULE_AFTO)
> + return EPFNOSUPPORT;
> + break;
> + case AF_INET:
> + if ((r->rule_flag & PFRULE_AFTO) && r->naf != AF_INET6)
> + return EPFNOSUPPORT;
> + break;
> +#ifdef INET6
> + case AF_INET6:
> + if ((r->rule_flag & PFRULE_AFTO) && r->naf != AF_INET)
> + return EPFNOSUPPORT;
> + break;
> +#endif /* INET6 */
> + default:
> + return EPFNOSUPPORT;
> + }
> +
> + if ((r->rule_flag & PFRULE_AFTO) == 0 && r->naf != 0)
> + return EPFNOSUPPORT;
> +
> + return 0;
> }
>
can we have 'return (EPFNOSUPPORT);' and 'return (0)' in
newly introduced `pf_rule_checkaf()` function? Just to keep
it consistent with remaining part of the file.
thanks and OK sashan
rpki-client MFT file and hash check change
This diff does a few things regarding MFT file and hash sequences:
- it validates the filename early on so that if considered valid it can
be printed by printf(%s) without problems.
- it assigns the file type (based on the file extension) early on and no
longer uses this information when comparing the file hash.
- Handle unknown files more like a soft error, the file hash still needs
to match but the content is totally ignored.
In other words it no longer rejects MFTs with unknown files in it.
Right now rpki-client is very strict in what is accepted and it will
become an some issue when ASPA is becomming more concrete.
--
:wq Claudio
Index: extern.h
===
RCS file: /cvs/src/usr.sbin/rpki-client/extern.h,v
retrieving revision 1.114
diff -u -p -r1.114 extern.h
--- extern.h23 Jan 2022 12:09:24 - 1.114
+++ extern.h24 Jan 2022 14:53:52 -
@@ -422,7 +422,6 @@ struct mft *mft_parse(X509 **, const cha
size_t);
struct mft *mft_read(struct ibuf *);
enum rtype rtype_from_file_extension(const char *);
-enum rtype rtype_from_mftfile(const char *);
voidroa_buffer(struct ibuf *, const struct roa *);
voidroa_free(struct roa *);
Index: main.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/main.c,v
retrieving revision 1.184
diff -u -p -r1.184 main.c
--- main.c 23 Jan 2022 18:40:55 - 1.184
+++ main.c 24 Jan 2022 15:05:59 -
@@ -376,7 +376,7 @@ queue_add_from_mft_set(const struct mft
case RTYPE_CRL:
continue;
default:
- logx("%s: unsupported file type: %s", name, f->file);
+ warnx("%s: unsupported file: %s", name, f->file);
}
}
}
Index: mft.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/mft.c,v
retrieving revision 1.50
diff -u -p -r1.50 mft.c
--- mft.c 22 Jan 2022 09:18:48 - 1.50
+++ mft.c 24 Jan 2022 14:50:40 -
@@ -152,22 +152,33 @@ rtype_from_file_extension(const char *fn
/*
* Validate that a filename listed on a Manifest only contains characters
- * permitted in draft-ietf-sidrops-6486bis section 4.2.2 and check that
- * it's a CER, CRL, GBR or a ROA.
- * Returns corresponding rtype or RTYPE_INVALID on error.
+ * permitted in draft-ietf-sidrops-6486bis section 4.2.2
*/
-enum rtype
-rtype_from_mftfile(const char *fn)
+static int
+valid_filename(const char *fn, size_t len)
{
- const unsigned char *c;
- enum rtype type;
+ const unsigned char *c;
+ size_t i;
- for (c = fn; *c != '\0'; ++c)
+ for (c = fn, i = 0; i < len; i++, c++)
if (!isalnum(*c) && *c != '-' && *c != '_' && *c != '.')
- return RTYPE_INVALID;
+ return 0;
- if (strchr(fn, '.') != strrchr(fn, '.'))
- return RTYPE_INVALID;
+ c = memchr(fn, '.', len);
+ if (c == NULL || c != memrchr(fn, '.', len))
+ return 0;
+
+ return 1;
+}
+
+/*
+ * Check that the file is a CER, CRL, GBR or a ROA.
+ * Returns corresponding rtype or RTYPE_INVALID on error.
+ */
+static enum rtype
+rtype_from_mftfile(const char *fn)
+{
+ enum rtype type;
type = rtype_from_file_extension(fn);
switch (type) {
@@ -217,15 +228,17 @@ mft_parse_filehash(struct parse *p, cons
p->fn, ASN1_tag2str(file->type), file->type);
goto out;
}
+ if (!valid_filename(file->value.ia5string->data,
+ file->value.ia5string->length)) {
+ warnx("%s: RFC 6486 section 4.2.2: bad filename", p->fn);
+ goto out;
+ }
fn = strndup((const char *)file->value.ia5string->data,
file->value.ia5string->length);
if (fn == NULL)
err(1, NULL);
- if ((type = rtype_from_mftfile(fn)) == RTYPE_INVALID) {
- warnx("%s: invalid filename: %s", p->fn, fn);
- goto out;
- }
+ type = rtype_from_mftfile(fn);
/* Now hash value. */
Index: parser.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/parser.c,v
retrieving revision 1.53
diff -u -p -r1.53 parser.c
--- parser.c23 Jan 2022 12:09:24 - 1.53
+++ parser.c24 Jan 2022 15:26:08 -
@@ -304,21 +304,13 @@ static int
proc_parser_mft_check(const char *fn, struct mft *p)
{
size_t i;
- int fd, try, rc = 1;
- char*h, *path;
+ int rc = 1;
+ char*path;
for (i = 0; i < p->filesz; i++) {
const struct mftfile *m = &p->files[i];
- if (rtype_from_mftfile(m->file) == RTYPE_INVALID) {
-
Re: unlock mmap(2) for anonymous mappings
On 24/01/22(Mon) 12:06, Klemens Nanni wrote:
> On Sun, Jan 16, 2022 at 09:22:50AM -0300, Martin Pieuchot wrote:
> > IMHO this approach of let's try if it works now and revert if it isn't
> > doesn't help us make progress. I'd be more confident seeing diffs that
> > assert for the right lock in the functions called by uvm_mapanon() and
> > documentation about which lock is protecting which field of the data
> > structures.
>
> I picked `vm_map's `size' as first underdocumented member.
> All accesses to it are protected by vm_map_lock(), either through the
> function itself or implicitly by already requiring the calling function
> to lock the map.
Could we use a vm_map_assert_locked() or something similar that reflect
the exclusive or shared (read) lock comments? I don't trust comments.
It's too easy to miss a lock in a code path.
> So annotate functions using `size' wrt. the required lock.
>
> Index: uvm_map.c
> ===
> RCS file: /cvs/src/sys/uvm/uvm_map.c,v
> retrieving revision 1.282
> diff -u -p -r1.282 uvm_map.c
> --- uvm_map.c 21 Dec 2021 22:21:32 - 1.282
> +++ uvm_map.c 21 Jan 2022 13:03:05 -
> @@ -805,6 +805,8 @@ uvm_map_sel_limits(vaddr_t *min, vaddr_t
> * Fills in *start_ptr and *end_ptr to be the first and last entry describing
> * the space.
> * If called with prefilled *start_ptr and *end_ptr, they are to be correct.
> + *
> + * map must be at least read-locked.
> */
> int
> uvm_map_isavail(struct vm_map *map, struct uvm_addr_state *uaddr,
> @@ -2206,6 +2208,8 @@ uvm_unmap_kill_entry(struct vm_map *map,
> * If remove_holes, then remove ET_HOLE entries as well.
> * If markfree, entry will be properly marked free, otherwise, no replacement
> * entry will be put in the tree (corrupting the tree).
> + *
> + * map must be locked.
> */
> void
> uvm_unmap_remove(struct vm_map *map, vaddr_t start, vaddr_t end,
> @@ -2976,6 +2980,9 @@ uvm_tree_sanity(struct vm_map *map, char
> UVM_ASSERT(map, addr == vm_map_max(map), file, line);
> }
>
> +/*
> + * map must be at least read-locked.
> + */
> void
> uvm_tree_size_chk(struct vm_map *map, char *file, int line)
> {
> Index: uvm_map.h
> ===
> RCS file: /cvs/src/sys/uvm/uvm_map.h,v
> retrieving revision 1.71
> diff -u -p -r1.71 uvm_map.h
> --- uvm_map.h 15 Dec 2021 12:53:53 - 1.71
> +++ uvm_map.h 21 Jan 2022 12:51:26 -
> @@ -272,7 +272,7 @@ struct vm_map {
>
> struct uvm_map_addr addr; /* [v] Entry tree, by addr */
>
> - vsize_t size; /* virtual size */
> + vsize_t size; /* [v] virtual size */
> int ref_count; /* [a] Reference count */
> int flags; /* flags */
> struct mutexflags_lock; /* flags lock */
>
syzcaller pf unhandled af
Hi,
https://syzkaller.appspot.com/bug?id=a6475751c2856d5ea5586f7120d14db1e00bf253
I think these crashes are caused by an af-to rule that has no
translation address family naf. Preventing such a rule in the
kernel might help.
ok?
bluhm
Index: net/pf_ioctl.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/net/pf_ioctl.c,v
retrieving revision 1.370
diff -u -p -r1.370 pf_ioctl.c
--- net/pf_ioctl.c 11 Jan 2022 09:00:17 - 1.370
+++ net/pf_ioctl.c 24 Jan 2022 12:41:40 -
@@ -109,6 +109,7 @@ void pf_trans_set_commit(void);
voidpf_pool_copyin(struct pf_pool *, struct pf_pool *);
int pf_validate_range(u_int8_t, u_int16_t[2]);
int pf_rule_copyin(struct pf_rule *, struct pf_rule *);
+int pf_rule_checkaf(struct pf_rule *);
u_int16_t pf_qname2qid(char *, int);
voidpf_qid2qname(u_int16_t, char *);
voidpf_qid_unref(u_int16_t);
@@ -1347,22 +1348,11 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a
rule = NULL;
break;
}
- switch (rule->af) {
- case 0:
- break;
- case AF_INET:
- break;
-#ifdef INET6
- case AF_INET6:
- break;
-#endif /* INET6 */
- default:
+ if ((error = pf_rule_checkaf(rule))) {
pf_rule_free(rule);
rule = NULL;
- error = EAFNOSUPPORT;
goto fail;
}
-
if (rule->src.addr.type == PF_ADDR_NONE ||
rule->dst.addr.type == PF_ADDR_NONE) {
error = EINVAL;
@@ -1601,23 +1591,11 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a
newrule = NULL;
break;
}
-
- switch (newrule->af) {
- case 0:
- break;
- case AF_INET:
- break;
-#ifdef INET6
- case AF_INET6:
- break;
-#endif /* INET6 */
- default:
- error = EAFNOSUPPORT;
+ if ((error = pf_rule_checkaf(newrule))) {
pf_rule_free(newrule);
newrule = NULL;
goto fail;
}
-
if (newrule->rt && !newrule->direction) {
pf_rule_free(newrule);
error = EINVAL;
@@ -3216,6 +3194,34 @@ pf_rule_copyin(struct pf_rule *from, str
to->set_prio[1] = from->set_prio[1];
return (0);
+}
+
+int
+pf_rule_checkaf(struct pf_rule *r)
+{
+ switch (r->af) {
+ case 0:
+ if (r->rule_flag & PFRULE_AFTO)
+ return EPFNOSUPPORT;
+ break;
+ case AF_INET:
+ if ((r->rule_flag & PFRULE_AFTO) && r->naf != AF_INET6)
+ return EPFNOSUPPORT;
+ break;
+#ifdef INET6
+ case AF_INET6:
+ if ((r->rule_flag & PFRULE_AFTO) && r->naf != AF_INET)
+ return EPFNOSUPPORT;
+ break;
+#endif /* INET6 */
+ default:
+ return EPFNOSUPPORT;
+ }
+
+ if ((r->rule_flag & PFRULE_AFTO) == 0 && r->naf != 0)
+ return EPFNOSUPPORT;
+
+ return 0;
}
int
cad(4): Disable extra queues
The GEMs on the PolarFire SoC implement priority queueing. Currently,
cad(4) does not initialize the extra queues, which causes a DMA error
on the first transmission. When the driver requests transmission, the
hardware begins checking the transmit queues from queue 3 down to
queue 0. The base addresses of queues 1-3 are in their reset state,
value 0. DMA fetch from physical address 0 fails.
The hardware does not seem to provide RSS-style hashing. The Rx queue
for an incoming frame is chosen based on a set of screener registers.
Each screener directs traffic to a particular queue if all the
screener's exact match conditions on frame input fields are satisfied.
Type 1 screeners allow exact matching based on the IPv4 TS or IPv6 TC
field and UDP destination port. Type 2 screeners match based on
EtherType, VLAN priority, and up to three flexible 32-bit or masked
16-bit fields.
The screeners could rig up a weak RSS "hash" for example by looking at
the least significant bits of the IP source and destination addresses.
However, I am not sure if this is worth the effort.
The Tx side does not look suitable for general multi-queueing.
A saturated high priority queue can starve a lower priority queue.
The following patch makes the driver disable the extra Rx and Tx queues.
This lets the driver work on the PolarFire SoC.
Some revisions of the GEM, such as PolarFire SoC's, have an explicit
option to disable unused queues. Other revisions need a dummy descriptor
that terminates an unused queue, at least on the Tx side. The patch uses
both disabling and termination. If GEM implements disabling, termination
should be harmless.
Someone should test this on a SiFive board with cad(4) to catch possible
mistakes.
OK?
Index: dev/fdt/if_cad.c
===
RCS file: src/sys/dev/fdt/if_cad.c,v
retrieving revision 1.8
diff -u -p -r1.8 if_cad.c
--- dev/fdt/if_cad.c29 Jul 2021 09:19:42 - 1.8
+++ dev/fdt/if_cad.c24 Jan 2022 13:35:30 -
@@ -1,7 +1,7 @@
/* $OpenBSD: if_cad.c,v 1.8 2021/07/29 09:19:42 patrick Exp $ */
/*
- * Copyright (c) 2021 Visa Hankala
+ * Copyright (c) 2021-2022 Visa Hankala
*
* Permission to use, copy, modify, and/or distribute this software for any
* purpose with or without fee is hereby granted, provided that the above
@@ -173,8 +173,22 @@
#define GEM_RXUDPCCNT 0x01b0
#define GEM_CFG6 0x0294
#define GEM_CFG6_DMA64(1 << 23)
+#define GEM_CFG6_PRIQ_MASK(x) ((x) & 0x)
+#define GEM_CFG8 0x029c
+#define GEM_CFG8_NUM_TYPE1_SCR(x) (((x) >> 24) & 0xff)
+#define GEM_CFG8_NUM_TYPE2_SCR(x) (((x) >> 16) & 0xff)
+#define GEM_TXQ1BASE(i)(0x0440 + (i) * 4)
+#define GEM_TXQ1BASE_DISABLE (1 << 0)
+#define GEM_RXQ1BASE(i)(0x0480 + (i) * 4)
+#define GEM_RXQ1BASE_DISABLE (1 << 0)
#define GEM_TXQBASEHI 0x04c8
#define GEM_RXQBASEHI 0x04d4
+#define GEM_SCR_TYPE1(i) (0x0500 + (i) * 4)
+#define GEM_SCR_TYPE2(i) (0x0540 + (i) * 4)
+#define GEM_RXQ8BASE(i)(0x05c0 + (i) * 4)
+#define GEM_RXQ8BASE_DISABLE (1 << 0)
+
+#define GEM_MAX_PRIQ 16
#define GEM_CLK_TX "tx_clk"
@@ -261,6 +275,9 @@ struct cad_softc {
unsigned char sc_rxdone;
unsigned char sc_dma64;
size_t sc_descsize;
+ uint32_tsc_qmask;
+ uint8_t sc_ntype1scr;
+ uint8_t sc_ntype2scr;
struct mii_data sc_mii;
#define sc_media sc_mii.mii_media
@@ -357,6 +374,7 @@ cad_match(struct device *parent, void *m
struct fdt_attach_args *faa = aux;
return (OF_is_compatible(faa->fa_node, "cdns,gem") ||
+ OF_is_compatible(faa->fa_node, "cdns,macb") ||
OF_is_compatible(faa->fa_node, "sifive,fu540-c000-gem") ||
OF_is_compatible(faa->fa_node, "sifive,fu740-c000-gem"));
}
@@ -370,6 +388,7 @@ cad_attach(struct device *parent, struct
struct ifnet *ifp = &sc->sc_ac.ac_if;
uint32_t hi, lo;
uint32_t rev, ver;
+ uint32_t val;
unsigned int i;
int node, phy;
@@ -426,10 +445,23 @@ cad_attach(struct device *parent, struct
ver = (rev & GEM_MID_VERSION_MASK) >> GEM_MID_VERSION_SHIFT;
sc->sc_descsize = sizeof(struct cad_desc32);
- /* Register CFG6 is not present on Zynq-7000 / GEM version 0x2. */
- if (ver >= 0x7 && (HREAD4(sc, GEM_CFG6) & GEM_CFG6_DMA64)) {
- sc->sc_descsize = sizeof(struct cad_desc64);
- sc->sc_dma64 = 1;
+ /* Queue 0 is always present. */
+ sc->sc_qmask = 0x1;
+ /*
+* Registers CFG1 and CFG6-10 are not present
+
Re: unlock mmap(2) for anonymous mappings
On Sun, Jan 16, 2022 at 09:22:50AM -0300, Martin Pieuchot wrote:
> IMHO this approach of let's try if it works now and revert if it isn't
> doesn't help us make progress. I'd be more confident seeing diffs that
> assert for the right lock in the functions called by uvm_mapanon() and
> documentation about which lock is protecting which field of the data
> structures.
I picked `vm_map's `size' as first underdocumented member.
All accesses to it are protected by vm_map_lock(), either through the
function itself or implicitly by already requiring the calling function
to lock the map.
So annotate functions using `size' wrt. the required lock.
Index: uvm_map.c
===
RCS file: /cvs/src/sys/uvm/uvm_map.c,v
retrieving revision 1.282
diff -u -p -r1.282 uvm_map.c
--- uvm_map.c 21 Dec 2021 22:21:32 - 1.282
+++ uvm_map.c 21 Jan 2022 13:03:05 -
@@ -805,6 +805,8 @@ uvm_map_sel_limits(vaddr_t *min, vaddr_t
* Fills in *start_ptr and *end_ptr to be the first and last entry describing
* the space.
* If called with prefilled *start_ptr and *end_ptr, they are to be correct.
+ *
+ * map must be at least read-locked.
*/
int
uvm_map_isavail(struct vm_map *map, struct uvm_addr_state *uaddr,
@@ -2206,6 +2208,8 @@ uvm_unmap_kill_entry(struct vm_map *map,
* If remove_holes, then remove ET_HOLE entries as well.
* If markfree, entry will be properly marked free, otherwise, no replacement
* entry will be put in the tree (corrupting the tree).
+ *
+ * map must be locked.
*/
void
uvm_unmap_remove(struct vm_map *map, vaddr_t start, vaddr_t end,
@@ -2976,6 +2980,9 @@ uvm_tree_sanity(struct vm_map *map, char
UVM_ASSERT(map, addr == vm_map_max(map), file, line);
}
+/*
+ * map must be at least read-locked.
+ */
void
uvm_tree_size_chk(struct vm_map *map, char *file, int line)
{
Index: uvm_map.h
===
RCS file: /cvs/src/sys/uvm/uvm_map.h,v
retrieving revision 1.71
diff -u -p -r1.71 uvm_map.h
--- uvm_map.h 15 Dec 2021 12:53:53 - 1.71
+++ uvm_map.h 21 Jan 2022 12:51:26 -
@@ -272,7 +272,7 @@ struct vm_map {
struct uvm_map_addr addr; /* [v] Entry tree, by addr */
- vsize_t size; /* virtual size */
+ vsize_t size; /* [v] virtual size */
int ref_count; /* [a] Reference count */
int flags; /* flags */
struct mutexflags_lock; /* flags lock */
Re: dt: make vmm tracepoints amd64 only
On Mon, Jan 17, 2022 at 04:51:37PM -0800, Philip Guenther wrote: > On Mon, Jan 17, 2022 at 5:02 AM Klemens Nanni wrote: > > > These don't hurt on !VMM architectures but I was still surprised to see > > them on e.g. sparc64: > > > > # arch -s ; btrace -l | grep vmm > > sparc64 > > tracepoint:vmm:guest_enter > > tracepoint:vmm:guest_exit > > > > Like some network drivers, we could use __amd64__ to limit those to > > amd64 and save a few bits in all other kernels. > > > > Is this approach feasible or should we just ignore such one-offs? > > > > If there's a non-trivial number of trace points in drivers, then it would > suggest that such tracepoints should be added/registered/pick-a-term by the > driver's attach routine. This needs a little bit of refactor around probe registration. I plan to commit the initial `#ifdef __amd64__' diff with "OK pv" soon unless I hear objections.
