CVS commit: src/sys

[Ryota Ozaki]

Module Name: src
Committed By: ozaki-r
Date: Fri Jul 17 02:21:08 UTC 2015

Modified Files:
src/sys/net: if.c route.c route.h rtsock.c
src/sys/netinet: if_atm.c ip_output.c
src/sys/netinet6: icmp6.c nd6.c nd6.h nd6_nbr.c nd6_rtr.c

Log Message:
Reform use of rt_refcnt

rt_refcnt of rtentry was used in bad manners, for example, direct rt_refcnt++
and rt_refcnt-- outside route.c, "rt->rt_refcnt++; rtfree(rt);" idiom, and
touching rt after rt->rt_refcnt--.

These abuses seem to be needed because rt_refcnt manages only references
between rtentry and doesn't take care of references during packet processing
(IOW references from local variables). In order to reduce the above abuses,
the latter cases should be counted by rt_refcnt as well as the former cases.

This change improves consistency of use of rt_refcnt:
- rtentry is always accessed with rt_refcnt incremented
- rtentry's rt_refcnt is decremented after use (rtfree is always used instead
of rt_refcnt--)
- functions returning rtentry increment its rt_refcnt (and caller rtfree it)

Note that rt_refcnt prevents rtentry from being freed but doesn't prevent
rtentry from being updated. Toward MP-safe, we need to provide another
protection for rtentry, e.g., locks. (Or introduce a better data structure
allowing concurrent readers during updates.)


To generate a diff of this commit:
cvs rdiff -u -r1.316 -r1.317 src/sys/net/if.c
cvs rdiff -u -r1.145 -r1.146 src/sys/net/route.c
cvs rdiff -u -r1.91 -r1.92 src/sys/net/route.h
cvs rdiff -u -r1.171 -r1.172 src/sys/net/rtsock.c
cvs rdiff -u -r1.34 -r1.35 src/sys/netinet/if_atm.c
cvs rdiff -u -r1.243 -r1.244 src/sys/netinet/ip_output.c
cvs rdiff -u -r1.170 -r1.171 src/sys/netinet6/icmp6.c
cvs rdiff -u -r1.164 -r1.165 src/sys/netinet6/nd6.c
cvs rdiff -u -r1.65 -r1.66 src/sys/netinet6/nd6.h
cvs rdiff -u -r1.108 -r1.109 src/sys/netinet6/nd6_nbr.c
cvs rdiff -u -r1.100 -r1.101 src/sys/netinet6/nd6_rtr.c

Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.


--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-...@muc.de

[Nick Hudson]

Module Name: src
Committed By: skrll
Date: Wed Jul 22 10:32:17 UTC 2015

Modified Files:
src/sys/arch/arm/amlogic: amlogic_dwctwo.c
src/sys/arch/arm/rockchip: rockchip_dwctwo.c
src/sys/arch/evbarm/conf: GENERIC.common
src/sys/arch/mips/cavium/dev: octeon_dwctwo.c
src/sys/dev: dksubr.c ld.c

Log Message:
Trailing whitespace.

To generate a diff of this commit:

cvs rdiff -u -r1.2 -r1.3 src/sys/arch/arm/amlogic/amlogic_dwctwo.c
cvs rdiff -u -r1.2 -r1.3 src/sys/arch/arm/rockchip/rockchip_dwctwo.c
cvs rdiff -u -r1.7 -r1.8 src/sys/arch/evbarm/conf/GENERIC.common
cvs rdiff -u -r1.3 -r1.4 src/sys/arch/mips/cavium/dev/octeon_dwctwo.c
cvs rdiff -u -r1.66 -r1.67 src/sys/dev/dksubr.c
cvs rdiff -u -r1.83 -r1.84 src/sys/dev/ld.c

[Maxime Villard]

Module Name: src
Committed By: maxv
Date: Fri Jul 24 13:02:52 UTC 2015

Modified Files:
src/sys/compat/common: kern_time_50.c vfs_syscalls_20.c
src/sys/compat/linux/common: linux_socket.c
src/sys/compat/linux32/common: linux32_socket.c
src/sys/compat/netbsd32: netbsd32_compat_50.c
src/sys/compat/ultrix: ultrix_fs.c
src/sys/kern: kern_ntptime.c kern_time.c kern_veriexec.c sys_lwp.c
vfs_syscalls.c
src/sys/miscfs/procfs: procfs_linux.c
src/sys/ufs/ffs: ffs_vfsops.c

Log Message:
Unused inits (harmless).

Found by Brainy.

To generate a diff of this commit:

cvs rdiff -u -r1.28 -r1.29 src/sys/compat/common/kern_time_50.c
cvs rdiff -u -r1.38 -r1.39 src/sys/compat/common/vfs_syscalls_20.c
cvs rdiff -u -r1.125 -r1.126 src/sys/compat/linux/common/linux_socket.c
cvs rdiff -u -r1.19 -r1.20 src/sys/compat/linux32/common/linux32_socket.c
cvs rdiff -u -r1.29 -r1.30 src/sys/compat/netbsd32/netbsd32_compat_50.c
cvs rdiff -u -r1.54 -r1.55 src/sys/compat/ultrix/ultrix_fs.c
cvs rdiff -u -r1.55 -r1.56 src/sys/kern/kern_ntptime.c
cvs rdiff -u -r1.179 -r1.180 src/sys/kern/kern_time.c
cvs rdiff -u -r1.8 -r1.9 src/sys/kern/kern_veriexec.c
cvs rdiff -u -r1.56 -r1.57 src/sys/kern/sys_lwp.c
cvs rdiff -u -r1.499 -r1.500 src/sys/kern/vfs_syscalls.c
cvs rdiff -u -r1.70 -r1.71 src/sys/miscfs/procfs/procfs_linux.c
cvs rdiff -u -r1.334 -r1.335 src/sys/ufs/ffs/ffs_vfsops.c

[Maxime Villard]

Module Name: src
Committed By: maxv

Date: Mon Jul 27 09:24:28 UTC 2015

Modified Files:
src/sys/kern: subr_kmem.c
src/sys/uvm: files.uvm
Removed Files:
src/sys/uvm: uvm_kmguard.c uvm_kmguard.h

Log Message:
Several changes and improvements in KMEM_GUARD:
- merge uvm_kmguard.{c,h} into subr_kmem.c. It is only user there, and
makes it more consistent. Also, it allows us to enable KMEM_GUARD
without enabling DEBUG.
- rename uvm_kmguard_XXX to kmem_guard_XXX, for consistency
- improve kmem_guard_alloc() so that it supports allocations bigger than
PAGE_SIZE
- remove the canary value, and use directly the kmem header as underflow
pattern.
- fix some comments

(The UAF fifo is disabled for the moment; we actually need to register
the va and its size, and add a weight support not to consume too much
memory.)

To generate a diff of this commit:

cvs rdiff -u -r1.60 -r1.61 src/sys/kern/subr_kmem.c
cvs rdiff -u -r1.24 -r1.25 src/sys/uvm/files.uvm
cvs rdiff -u -r1.11 -r0 src/sys/uvm/uvm_kmguard.c
cvs rdiff -u -r1.2 -r0 src/sys/uvm/uvm_kmguard.h

[matthew green]

"Maxime Villard" writes:
> Module Name: src
> Committed By: maxv
> Date: Mon Jul 27 09:24:28 UTC 2015
>
> Modified Files:
> src/sys/kern: subr_kmem.c
> src/sys/uvm: files.uvm
> Removed Files:
> src/sys/uvm: uvm_kmguard.c uvm_kmguard.h
>
> Log Message:
> Several changes and improvements in KMEM_GUARD:
> - merge uvm_kmguard.{c,h} into subr_kmem.c. It is only user there, and
> makes it more consistent. Also, it allows us to enable KMEM_GUARD
> without enabling DEBUG.
> - rename uvm_kmguard_XXX to kmem_guard_XXX, for consistency
> - improve kmem_guard_alloc() so that it supports allocations bigger than
> PAGE_SIZE
> - remove the canary value, and use directly the kmem header as underflow
> pattern.
> - fix some comments
>
> (The UAF fifo is disabled for the moment; we actually need to register
> the va and its size, and add a weight support not to consume too much
> memory.)

thanks for extending KMEM_GUARD beyond PAGE_SIZE. this will be
quite helpful, if even now requiring a lot more memory when
using kmguard :)

was this change presented for review anywhere before commit?

i'm not convinced that moving directly into subr_kmem.c is the
right thing or the only way to solve the problem of depending
on DEBUG. could you separate them again, please?

the failure mode in the new kmem_guard_alloc() is also expensive,
in that instead of knowing how to unwind itself, it calls into
UVM to figure out all the details (which involves pmap_extract()
and a bunch more accounting.) it would probably be best to
replace this with a call to uvm_pglistalloc() in the new version,
and then you'll never have a partial allocation failure to deal
with and you only make a single call to allocate these pages.

also, about removing the canary -- can you explain this a little
more, i'm not sure i understand this part.

thanks.


.mrg.

[Maxime Villard]

Not really. In fact, I don't really understand why the comment in
subr_kmem's header says it is "very expensive", since even on my
slow laptop VM, I see no performance regression with KMEM_GUARD
enabled.

It is true that, now that the allocations can take several pages, the
fifo (which only holds the original va) can queue a lot of memory. My
plan was to add a field in the guard structure in order to hold the
weight of the fifo, and not insert enormous elements into it.

I also planned to always enable KMEM_GUARD on DEBUG, even when depth=0.
depth is only used for the fifo; having no fifo does not prevent the
guard page from being here and working. And then, depending on how much
memory you have, you can modify that value to more or less detect UAFs..

>
> was this change presented for review anywhere before commit?
>
> i'm not convinced that moving directly into subr_kmem.c is the
> right thing or the only way to solve the problem of depending
> on DEBUG. could you separate them again, please?

I'm not convinced keeping them separate really makes sense; as I
said, it is kmem-specific, and gathering it with the kmem code
reduces fragmentation (and, having to hardcode things when you
want to enable it without enabling DEBUG). It was also a way to
replace the canary by the kmem header.

>
> the failure mode in the new kmem_guard_alloc() is also expensive,
> in that instead of knowing how to unwind itself, it calls into
> UVM to figure out all the details (which involves pmap_extract()
> and a bunch more accounting.) it would probably be best to
> replace this with a call to uvm_pglistalloc() in the new version,
> and then you'll never have a partial allocation failure to deal
> with and you only make a single call to allocate these pages.

Yes, probably. But I'm more concerned about fixing the fifo.

Actually, we need to register the va and its size into the fifo, and I
haven't found an atomic function that can modify two pointers.

>
> also, about removing the canary -- can you explain this a little
> more, i'm not sure i understand this part.

Just give a look at subr_kmem's header:

* |CHUNK|CHUNK|CHUNK|CHUNK|CHUNK|CHUNK|CHUNK|CHUNK|CHUNK|CHUNK|CHUNK|
* +-----+-----+-----+-----+-----+-----+-----+-----+-----+---+-+--+--+
* |/////| | | | | | | | | |*|**|UU|
* |/HSZ/| | | | | | | | | |*|**|UU|
* |/////| | | | | | | | | |*|**|UU|
* +-----+-----+-----+-----+-----+-----+-----+-----+-----+---+-+--+--+
* |Size | Buffer usable by the caller (requested size) |RedZ|Unused\

The same applies to kmem_guard_alloc(): right before the returned
pointer, there's a kmem_header structure that indicates the size of the
allocated memory; the kernel compares this size with the one given in
kmem_free().

This header can therefore be used as an underflow pattern: if the caller
writes at p[-1], the size will be modified, and when freeing the kernel
will detect the mismatch.

But I know there's still an issue here:

struct kmem_header {
size_t size;
} __aligned(KMEM_ALIGN);

KMEM_ALIGN being 8, on 32bit systems, a padding is added by the compiler
in the last 4 bytes of the structure; which means that if the caller
writes at p[-1], the kernel won't detect it.

I guess there's a magic compiler option somewhere that can instruct the
compiler to pad at the bottom (so that the 'size' field sleeps at the
end of the structure).

[David A. Holland]

Module Name: src
Committed By: dholland
Date: Tue Jul 28 05:13:14 UTC 2015

Modified Files:
src/sys/lib/libsa: ufs.c
src/sys/ufs/lfs: lfs.h

Log Message:
Move struct salfs back inside libsa now that lfs_accessors.h is separate.

To generate a diff of this commit:

cvs rdiff -u -r1.68 -r1.69 src/sys/lib/libsa/ufs.c
cvs rdiff -u -r1.166 -r1.167 src/sys/ufs/lfs/lfs.h

Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.

[Maxime Villard]

Module Name: src
Committed By: maxv
Date: Tue Jul 28 12:32:44 UTC 2015

Modified Files:
src/sys/kern: subr_pool.c
src/sys/sys: pool.h

Log Message:
Introduce POOL_REDZONE.

To generate a diff of this commit:

cvs rdiff -u -r1.203 -r1.204 src/sys/kern/subr_pool.c
cvs rdiff -u -r1.77 -r1.78 src/sys/sys/pool.h

[Christos Zoulas]

In article <17847.14...@splode.eterna.com.au>,

While the technical part of this change has been resolved, large
changes like this should be reviewed in tcsh-kern before they are
committed.

christos

[Christos Zoulas]

In article <mp8992$86m$1...@ger.gmane.org>,

Christos Zoulas <chri...@astron.com> wrote:
>
>While the technical part of this change has been resolved, large
>changes like this should be reviewed in tcsh-kern before they are
>committed.

"tech-kern"