Page MenuHomeFreeBSD

asomers (Alan Somers)
User

Projects

User Details

User Since
May 9 2014, 11:04 PM (331 w, 6 d)

Recent Activity

Fri, Sep 11

asomers added a comment to D26243: Add audit(4) support to NFS(v3).
In D26243#587132, @mjg wrote:

Audit support for regular lookup starts with AUDIT_ARG_UPATH1_VP/AUDIT_ARG_UPATH2_VP without any vnodes locked. Later on visited vnodes get added with AUDIT_ARG_VNODE1/AUDIT_ARG_VNODE2 which only performs VOP_GETATTR (i.e. does *NOT* resolve any paths). Your code should follow the same scheme.

As you can see path resolving routines can take vnode locks on their own (modulo the smr case). This means they can't be called with locked vnodes to begin with, as otherwise you risk violating global lock ordering and consequently deadlocking the kernel.

The VOP_ISLOCKED routine is not entirely legal to call if you don't hold the lock. The name is perhaps misleading, but it can only reliably tell you that you have an exclusive lock or that *SOMEONE* has a shared lock (and it may be you). Or to put it differently, if you don't have the vnode locked but someone else has it shared locked, you will get non-0 and that's how you get the panic. Regardless of this problem, adding the call reduces performance and most notably suggests a bug on its own.

So the question is why are you calling here with any vnodes locked.

I wish to audit the canonical path of the file requested by the NFS clients. The requested path from the client is extracted in the NFS server using nfsrv_parsename, but the vnode is locked in some NFS services. I thought of unlocking/relocking of vnode for path audit but Rick advised not to. That's why I had to call this locked vnode.

Thanks for your question which made me rethink the problem from scratch and I got a new idea for auditing path.

Hi @rmacklem and @asomers, if I use nfsvno_namei to get the canonical path for the client, I will not the need the AUDIT_ARG_UPATH1_VP.which will save me from all the trouble of passing locked vnode to vn_fullpath_global. Please provide your opinion on the same.

Fri, Sep 11, 11:11 PM · security, GSoC Students, Audit
asomers added a comment to D26204: fix integer underflow in getgrnam_r and getpwnam_r.

ping @pi @trasz . Are either of you able to review this change?

Fri, Sep 11, 10:57 PM
asomers committed rS365643: cp: fall back to read/write if copy_file_range fails.
cp: fall back to read/write if copy_file_range fails
Fri, Sep 11, 8:49 PM
asomers closed D26395: cp: fall back to read/write if copy_file_range fails.
Fri, Sep 11, 8:49 PM

Thu, Sep 10

asomers added inline comments to D26395: cp: fall back to read/write if copy_file_range fails.
Thu, Sep 10, 6:20 PM
asomers requested review of D26395: cp: fall back to read/write if copy_file_range fails.
Thu, Sep 10, 6:11 PM
asomers committed rS365549: cp: use copy_file_range(2).
cp: use copy_file_range(2)
Thu, Sep 10, 2:49 AM
asomers closed D26377: cp: use copy_file_range(2).
Thu, Sep 10, 2:49 AM
asomers added a comment to D26377: cp: use copy_file_range(2).

Yeah, doing copies 2MB at a time shouldn't be too slow. However, it does cause a different problem, I think. When I used this to copy a 16 GB sparse file (512B used) on UFS, the output file had 32MB used. I'm guessing that's due to UFS indirect blocks, and that increasing the copy_file_range bufsize would reduce the number of indirect blocks created. Alternatively (and probably better) would be for cp to use SEEK_HOLE/SEEK_DATA to completely skip the holes. But this version is still a lot better than the previous one.

Thu, Sep 10, 2:47 AM

Wed, Sep 9

asomers requested review of D26377: cp: use copy_file_range(2).
Wed, Sep 9, 4:07 PM
asomers committed rS365503: MFC r365262:.
MFC r365262:
Wed, Sep 9, 1:16 PM

Tue, Sep 8

asomers accepted D26361: Don't return errors from the cryptodev_process() method..
Tue, Sep 8, 5:44 PM

Mon, Sep 7

asomers requested changes to D26243: Add audit(4) support to NFS(v3).

The new code looks better. But grrr, there are two big problems:

  1. It doesn't compile due to some recent changes on head. I suggest the following:
    • Remove the <rpc/rpc.h>, <sys/mount.h>, and <fs/nfs/*> includes from audit.h. In addition to fixing the compile failure, it's generally not recommended to include headers from other headers. Sometimes it's necessary, but it also causes header pollution, and slow build times. Instead of including those files, just forward declare struct nfsrv_descript; and struct kaudit_record;.
    • Add `<netinet/in.h>, <rpc/rpc.h>, <fs/nfs/nfsdport.h>, <fs/nfs/nfsproto.h>, and <fs/nfs/nfs.h> to audit_bsm_db.c
    • Add <rpc/rpc.h>, <fs/nfs/nfsport.h>, <fs/nfs/nfsproto.h>, and <fs/nfs/nfs.h> to audit.c
Mon, Sep 7, 5:30 PM · security, GSoC Students, Audit
asomers committed rS365415: nsswitch.conf.5: style fixes.
nsswitch.conf.5: style fixes
Mon, Sep 7, 1:45 PM
asomers closed D26345: nsswitch.conf.5: style fixes.
Mon, Sep 7, 1:45 PM

Sun, Sep 6

asomers added inline comments to D26243: Add audit(4) support to NFS(v3).
Sun, Sep 6, 9:05 PM · security, GSoC Students, Audit
asomers requested review of D26345: nsswitch.conf.5: style fixes.
Sun, Sep 6, 8:35 PM
asomers committed rS365391: nsswitch.conf(5): recommend placing cache after files.
nsswitch.conf(5): recommend placing cache after files
Sun, Sep 6, 8:32 PM
asomers closed D26184: nsswitch.conf(5): recommend placing cache after files.
Sun, Sep 6, 8:32 PM
asomers added a comment to D26184: nsswitch.conf(5): recommend placing cache after files.
In D26184#581938, @se wrote:

I have added the configuration option "negative-confidence-threshold" to nscd in r238094 (in 2012), but since the default was kept at "1", this change has not become visible in practice.

It should work (did at that time in my tests), but I have received neither negative nor positive reports of its fitness for the intended purpose.

A value of 2 for this parameter ought to prevent the issue of entries being probed, not found, added, and then still not found due to the negative cache entry from the first probe.

This adds the cost of one additional negative lookup for each non-existent entry, which might be or might be not preferable to first looking up the file.

But it should help if new entries are added to other database types than local files (e.g., to NIS), a case not covered by the suggested change to the lookup order. (And having "cache" behind "nis" would negate its usefulness.)

I'm not opposed to changing the order from "cache files" to "files cache" (assuming that the file being read is on a local file system, not on e.g. NFS in case of a disk-less client), but I'd want to suggest that the "negative-confidence-threshold" was tested (in different settings) and proposed as a possible solution for this use case, too.

Sun, Sep 6, 7:45 PM
asomers committed rS365389: padlock(4): fix instapanics with geli authentication.
padlock(4): fix instapanics with geli authentication
Sun, Sep 6, 7:25 PM
asomers accepted D26342: Add a check for LCL_NEEDSCONFIRM to nfsrv_checkgetattr().

If the RFC does not allow unconfirmed clients to have delegations, then I think this change is correct. And I can't find any other problems with it.

Sun, Sep 6, 7:12 PM
asomers added a comment to D26342: Add a check for LCL_NEEDSCONFIRM to nfsrv_checkgetattr().

If it makes a difference, one of the crashed server's clients _did_ recently do a change of address (though I'm unsure of the exact timing, and of course I can't guarantee that the client isn't buggy). But none of the clients rebooted within 4 days of the crash.

Sun, Sep 6, 3:19 AM

Sat, Sep 5

asomers added a comment to D26342: Add a check for LCL_NEEDSCONFIRM to nfsrv_checkgetattr().

This seems wrong for two reasons:

  • nfsrv_docallback has other callers, too
  • How can we be sure that the previous client isn't still using its delegation?
Sat, Sep 5, 11:37 PM

Wed, Sep 2

asomers committed rS365262: Fix output of nfsstat -cE in json or xml mode.
Fix output of nfsstat -cE in json or xml mode
Wed, Sep 2, 5:36 PM

Sun, Aug 30

asomers added a comment to D25869: Add audit(4) support to NFS(v3).

It looks like your most recent change rebased the base revision. That makes it very hard to see which changes are from you and which aren't. Could you please either un-rebase it or, if that's not possible, open a new review?

Ohh, Sorry! I didn't thought pulling HEAD changes will create this side-effect in revision
I think I would open a new review as going back will have conflicting changes again. Should I abandon this while creating a new one??

Sun, Aug 30, 8:58 PM · security, GSoC Students, Audit
asomers committed rS364978: MFC r364412:.
MFC r364412:
Sun, Aug 30, 6:22 PM
asomers committed rS364974: MFC r364412:.
MFC r364412:
Sun, Aug 30, 4:28 PM
asomers added a comment to D25869: Add audit(4) support to NFS(v3).

Using two completely separate functions reduces the scope of error. Also prevent any mutation to the current code path for not locked vnodes, while allowing it to work for locked vnodes.

Sun, Aug 30, 2:54 PM · security, GSoC Students, Audit

Wed, Aug 26

asomers requested review of D26204: fix integer underflow in getgrnam_r and getpwnam_r.
Wed, Aug 26, 7:30 PM
asomers committed rS364800: geli: use unmapped I/O.
geli: use unmapped I/O
Wed, Aug 26, 2:45 AM
asomers committed rS364799: crypto(9): add CRYPTO_BUF_VMPAGE.
crypto(9): add CRYPTO_BUF_VMPAGE
Wed, Aug 26, 2:38 AM
asomers closed D25671: geli: use unmapped I/O.
Wed, Aug 26, 2:38 AM

Tue, Aug 25

asomers requested review of D26184: nsswitch.conf(5): recommend placing cache after files.
Tue, Aug 25, 10:59 PM
asomers committed rP546235: net/nbdkit: fix the GNUTLS option.
net/nbdkit: fix the GNUTLS option
Tue, Aug 25, 10:38 PM

Sun, Aug 23

asomers updated the diff for D25671: geli: use unmapped I/O.

Fix build on powerpc and fix one style bug

Sun, Aug 23, 2:20 PM

Thu, Aug 20

asomers committed rP545486: MFH: 544720.
MFH: 544720
Thu, Aug 20, 2:01 AM
asomers committed rP545482: sysutils/zfsnap2: must install periodic script in relevant directory..
sysutils/zfsnap2: must install periodic script in relevant directory.
Thu, Aug 20, 1:37 AM
asomers committed rS364412: zfs: fix EIO accessing dataset after resuming interrupted receive.
zfs: fix EIO accessing dataset after resuming interrupted receive
Thu, Aug 20, 1:31 AM
asomers closed D26034: zfs: fix EIO accessing dataset after resuming interrupted receive.
Thu, Aug 20, 1:31 AM
asomers added a comment to D26034: zfs: fix EIO accessing dataset after resuming interrupted receive.

LGTM

Thu, Aug 20, 1:31 AM
asomers added a comment to D26034: zfs: fix EIO accessing dataset after resuming interrupted receive.

ping. Any comments here? I'd like to commit before 12.2-RELEASE if possible.

Thu, Aug 20, 1:13 AM
asomers added a comment to D25671: geli: use unmapped I/O.

ping. Any further comments on this PR?

Thu, Aug 20, 1:11 AM

Aug 19 2020

asomers added a comment to D25869: Add audit(4) support to NFS(v3).

This is a much better locking strategy. However, there's a lot of duplicated code. Could you maybe combine the _locked with the original functions, so there wouldn't be so much duplication?

Aug 19 2020, 2:43 AM · security, GSoC Students, Audit

Aug 14 2020

asomers updated the diff for D25671: geli: use unmapped I/O.

Fix crypto_cursor_advance when amount > 4096

Aug 14 2020, 2:04 AM
asomers added inline comments to D25671: geli: use unmapped I/O.
Aug 14 2020, 2:04 AM

Aug 12 2020

asomers updated the diff for D25671: geli: use unmapped I/O.
  • Update man pages
Aug 12 2020, 11:09 PM
asomers added a comment to D25671: geli: use unmapped I/O.
In D25671#577993, @jhb wrote:

Somehow my earlier comment was stuck. There are still some style nits about leading spaces before block comments (and before the new "skip" macro in criov.c). I think the general change looks good. I think it would be good to commit the new buffer type separately from the geli changes (so two commits). I will probably test this for KTLS in the near future.

Aug 12 2020, 11:04 PM
asomers committed rP544757: devel/cdialog: Update to 1.3-20200327.
devel/cdialog: Update to 1.3-20200327
Aug 12 2020, 4:59 PM

Aug 11 2020

asomers committed rP544720: sysutils/openzfs-kmod: fix the build with a nonstandard SRC_BASE.
sysutils/openzfs-kmod: fix the build with a nonstandard SRC_BASE
Aug 11 2020, 10:25 PM
asomers closed D26033: sysutils/openzfs-kmod: fix the build with a nonstandard SRC_BASE.
Aug 11 2020, 10:25 PM
asomers updated the diff for D26033: sysutils/openzfs-kmod: fix the build with a nonstandard SRC_BASE.

Don't duplicate USE=kmod's IGNORE warning

Aug 11 2020, 8:52 PM
asomers updated subscribers of D26033: sysutils/openzfs-kmod: fix the build with a nonstandard SRC_BASE.

Shouldn't the USES=kmod bit handle this? Mk/Uses/kmod.mk

Aug 11 2020, 8:51 PM
asomers requested review of D26034: zfs: fix EIO accessing dataset after resuming interrupted receive.
Aug 11 2020, 8:36 PM
asomers requested review of D26033: sysutils/openzfs-kmod: fix the build with a nonstandard SRC_BASE.
Aug 11 2020, 8:20 PM
asomers added a comment to D25671: geli: use unmapped I/O.

ping. Is there anything else I need to do here?

Aug 11 2020, 2:21 PM
asomers committed rS364094: fusefs: fix the FUSE_FORGET unit test after r364064.
fusefs: fix the FUSE_FORGET unit test after r364064
Aug 11 2020, 1:09 AM

Aug 10 2020

asomers committed rP544646: net/libnfs: fix build error regarding "redefinition of typedef".
net/libnfs: fix build error regarding "redefinition of typedef"
Aug 10 2020, 8:49 PM

Aug 8 2020

asomers committed rP544532: sysutils/py-salt: revert upstream PR 55719.
sysutils/py-salt: revert upstream PR 55719
Aug 8 2020, 11:44 PM

Aug 7 2020

asomers accepted D25663: Fix linker error in libuutil with recent LLVM.

Ok. As long as you've tested it with GCC6, then it's fine by me.

Aug 7 2020, 2:49 PM
asomers added a comment to D25663: Fix linker error in libuutil with recent LLVM.

r329984 was done for a reason. With your change, can you still build world with GCC6?

Aug 7 2020, 2:32 PM

Jul 30 2020

asomers committed rP543809: astro/xearth: Purge outdated accounts from freebsd.committers.markers.
astro/xearth: Purge outdated accounts from freebsd.committers.markers
Jul 30 2020, 5:22 PM
asomers closed D25889: astro/xearth: Purge outdated accounts from freebsd.committers.markers.
Jul 30 2020, 5:22 PM
asomers updated the diff for D25889: astro/xearth: Purge outdated accounts from freebsd.committers.markers.

Restore gordon and knu

Jul 30 2020, 1:40 PM
asomers added a comment to D25889: astro/xearth: Purge outdated accounts from freebsd.committers.markers.

Thanks for catching those. My process is only partially automated.

Jul 30 2020, 1:40 PM
asomers requested review of D25889: astro/xearth: Purge outdated accounts from freebsd.committers.markers.
Jul 30 2020, 3:22 AM
asomers committed rP543703: astro/xearth: add myself (asomers) to freebsd.committers.markers.
astro/xearth: add myself (asomers) to freebsd.committers.markers
Jul 30 2020, 2:41 AM

Jul 29 2020

asomers added inline comments to D25869: Add audit(4) support to NFS(v3).
Jul 29 2020, 7:19 PM · security, GSoC Students, Audit
asomers added inline comments to D25869: Add audit(4) support to NFS(v3).
Jul 29 2020, 12:10 AM · security, GSoC Students, Audit

Jul 27 2020

asomers committed rS363622: Restrict definition of CTL_P1003_1B_MAXID to the kernel.
Restrict definition of CTL_P1003_1B_MAXID to the kernel
Jul 27 2020, 6:57 PM
asomers closed D25816: Restrict definition of CTL_P1003_1B_MAXID to the kernel.
Jul 27 2020, 6:57 PM

Jul 26 2020

asomers requested review of D25816: Restrict definition of CTL_P1003_1B_MAXID to the kernel.
Jul 26 2020, 4:55 PM

Jul 24 2020

asomers committed rS363486: MFC r363014:.
MFC r363014:
Jul 24 2020, 6:19 PM
asomers committed rS363485: MFC r362891:.
MFC r362891:
Jul 24 2020, 5:56 PM
asomers committed rS363484: MFC r362891:.
MFC r362891:
Jul 24 2020, 5:45 PM

Jul 22 2020

asomers updated the diff for D25671: geli: use unmapped I/O.

Use CRYPTO_BUF_VMPAGE instead of CRYPTO_BUF_UNMAPPED, and name static functions cvm_page_XXX

Jul 22 2020, 7:09 PM
asomers updated the diff for D25671: geli: use unmapped I/O.

Minimize conditional compilation, and style fix to "char*"

Jul 22 2020, 5:34 PM
asomers added a comment to D25671: geli: use unmapped I/O.
  • Avoid conditionally defining struct members and code when possible, especially in headers. This removes the need to import machine/param.h in crypto headers.
Jul 22 2020, 2:35 PM
asomers updated the diff for D25671: geli: use unmapped I/O.

Respond to kib's comments, mostly style. Also, rebase.

Jul 22 2020, 2:36 AM

Jul 21 2020

asomers added inline comments to D25671: geli: use unmapped I/O.
Jul 21 2020, 10:31 PM
asomers committed rS363402: Fix geli's null cipher, and add a test case.
Fix geli's null cipher, and add a test case
Jul 21 2020, 7:18 PM
asomers committed rS363398: [skip ci] document close_range(2) as async-signal-safe.
[skip ci] document close_range(2) as async-signal-safe
Jul 21 2020, 4:47 PM
asomers closed D25513: [skip ci] document close_range(2) as async-signal-safe.
Jul 21 2020, 4:47 PM
asomers updated the diff for D25671: geli: use unmapped I/O.

Conditionally compile most CRYPTO_BUF_VMPAGE code

Jul 21 2020, 4:44 PM

Jul 20 2020

asomers requested review of D25747: geli: use a single crypto session for each provider.
Jul 20 2020, 10:51 PM
asomers updated the diff for D25671: geli: use unmapped I/O.

Allow PMAP_HAS_DMAP to change at runtime

Jul 20 2020, 9:52 PM
asomers added inline comments to D25671: geli: use unmapped I/O.
Jul 20 2020, 9:28 PM
asomers added a comment to D25671: geli: use unmapped I/O.
In D25671#569908, @kib wrote:

Now I am confused. It seems that the new patch works on the page at the time, unless I missed a detail. Then, if we return to the design where you used sf bufs, you actually do not need more then one sf_buf at the time. I am not stating that it is possible to changing the patch back to sf_bufs, since it might be impossible to use sleepable allocation, in which case you would need to retract to transient mapping. But if this is not issue, then 'single sf buf at a time' might be an option.

Jul 20 2020, 9:03 PM
asomers updated the diff for D25671: geli: use unmapped I/O.

Use a vm_page_t array instead of sf_buf, only on DMAP architectures.

Jul 20 2020, 5:57 PM
asomers committed rS363368: padlock: fix Via Padlock with 192-bit keys.
padlock: fix Via Padlock with 192-bit keys
Jul 20 2020, 4:12 PM
asomers closed D25710: padlock: fix Via Padlock with 192-bit keys.
Jul 20 2020, 4:12 PM
asomers committed rS363361: tests/sys/opencrypto: use python3.
tests/sys/opencrypto: use python3
Jul 20 2020, 12:47 PM
asomers closed D25682: tests/sys/opencrypto: use python3.
Jul 20 2020, 12:47 PM

Jul 19 2020

asomers added inline comments to D25671: geli: use unmapped I/O.
Jul 19 2020, 4:19 AM

Jul 18 2020

asomers updated the diff for D25671: geli: use unmapped I/O.

Fix deadlock during out-of-sf_buf condition

Jul 18 2020, 4:14 PM
asomers added inline comments to D25671: geli: use unmapped I/O.
Jul 18 2020, 2:06 AM

Jul 17 2020

asomers added inline comments to D25671: geli: use unmapped I/O.
Jul 17 2020, 11:36 PM
asomers updated the diff for D25671: geli: use unmapped I/O.

Add sf_buf intelligence to crypto(9) and use it for geli reads

Jul 17 2020, 10:56 PM
asomers requested review of D25710: padlock: fix Via Padlock with 192-bit keys.
Jul 17 2020, 6:21 PM
asomers added a comment to D25671: geli: use unmapped I/O.
In D25671#568830, @kib wrote:

Ok, you two successfully guilt tripped me. I went ahead and did it the way you suggested, adding sf_buf intelligence to crypto(9) and using it for reads (but still using uiomove for writes). It does improve read performance with larger block sizes. But curiously it only works when the aesni module is loaded. Without that module, using pure software crypto, I get miscompares on every read. I'm betting that one of you two already knows the answer why. Could you fill me in?

I cannot debug the code by description of it.

Jul 17 2020, 1:01 PM