Details

Reviewers

markj
mjg

Commits

rGea2b64c24133: ktrace: add a kern.ktrace.filesize_limit_signal knob
rG02645b886bc6: ktrace: use the limit of the trace initiator for file size limit on writes
rG1762f674ccb5: ktrace: pack all ktrace parameters into allocated structure ktr_io_params
rGa6144f713cee: ktrace: do not stop tracing other processes if our cannot write to this vnode
rG9bb84c23e762: accounting: explicitly mark the exiting thread as doing accounting
rG70c05850e276: kern_descrip.c: Style

Summary

Use the mark to stop applying file size limits on the write of the accounting record. This allows to remove hack to clear process limits in acct_process(), and avoids the bug with the clearing being ineffective because limits are also cached in the thread structure.

Use proper file size limit from the ktrace requester for ktrace record writes.

Reported by: markj

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

kib requested review of this revision.May 13 2021, 11:57 PM

kib created this revision.

kib mentioned this in D30252: acct: Reset the thread rlimits on exit.

markj accepted this revision.May 14 2021, 1:35 AM

This revision is now accepted and ready to land.May 14 2021, 1:35 AM

I disagree with this method. There are other cases where rlimits gets in the way of correct operation, for example ktrace: rlimit of the target is used when it should not be to limit the output of the kernel log (instead, if any limit is to be there, it should be taken from the process which instigated ktracing). I suspect the right way to fix this is to swap around the rlimit around ktrace calls and same technique can be used here, similarly to how the original patch tried to do it.

In D30257#679628, @mjg wrote:

I disagree with this method. There are other cases where rlimits gets in the way of correct operation, for example ktrace: rlimit of the target is used when it should not be to limit the output of the kernel log (instead, if any limit is to be there, it should be taken from the process which instigated ktracing). I suspect the right way to fix this is to swap around the rlimit around ktrace calls and same technique can be used here, similarly to how the original patch tried to do it.

I disagree with your objections. It is not correct to swap neither credentials nor limits for the thread. VOP operations, all some parallel actors, might examine and use them for something not indended. [I am aware that NFS client sometimes swaps credentials]

There, for accounting, the solution is exact, RLIMIT_FSIZE should not be used for accounting write. Hack with resetting thread limit to not effective is a hack. It is probably less important for exiting process. Swapping limits for ktrace records writes is not acceptable, I think.

In fact, this approach, with explicit mark of the regions where vn_rlimit_fsize() should make special checks instead of the check against current thread' limits, works quite well for the ktrace writes too. I decided to refactor code to put the ktrace io related data into single structure, it is more convenient this way.

Add ktrace limit handling.
Refactor process ktrace io data, put it into dedicated structure.

This revision now requires review to proceed.May 15 2021, 1:04 AM

markj added inline comments.May 17 2021, 4:42 PM

sys/kern/kern_descrip.c
83–84	Sort?
sys/kern/kern_ktrace.c
441 ↗	(On Diff #89252)	`ktr_io_params_rele` would be a more standard name.
469 ↗	(On Diff #89252)	These two lines can be `res->cr = crhold(td->td_ucred)`.
488 ↗	(On Diff #89252)	I think it is not guaranteed in all cases that `p->p_ktrioparams` is not NULL. For instance, ktrace(KTROP_CLEAR) on a process which did not have tracing enabled before.
505 ↗	(On Diff #89252)
1042 ↗	(On Diff #89252)	Need to check `p->p_ktrioparms != NULL` first.
sys/kern/vfs_vnops.c
2366 ↗	(On Diff #89252)	I would use local variables for both sides of the `<=` comparison. This is painful to read otherwise.

markj added inline comments.May 17 2021, 4:47 PM

sys/kern/kern_ktrace.c
1125 ↗	(On Diff #89252)	Shouldn't the mutex be released first?

kib marked 8 inline comments as done.May 18 2021, 12:52 AM

kib added inline comments.

sys/kern/kern_descrip.c
83–84	Unfortunately no. ktrace.h is not self contained, the trace structures basically gather a lot of stuff from misc. syscalls declarations. And if some things like enum uio_rw can be handled by inclusion on sys/_uio.h, others e.g. sig_t cannot be handled reasonable except inclusion of signalvar.h before ktrace.h. So this header is outlier.
sys/kern/kern_ktrace.c
488 ↗	(On Diff #89252)	Yes, _unref/_rele should handle NULL arg, same as ktr_io_params_free(). I missed this.

_unref->_rele
fix unlocking/free order bug
Check for NULL, also s/0/NULL

markj added inline comments.May 18 2021, 2:03 PM

sys/kern/vfs_vnops.c
2373 ↗	(On Diff #89383)	So if a tracing process has a limit imposed, and tracing exceeds the limit, we will send a default-fatal signal to the traced process. This is not good behaviour for a debugging tool IMO. At this point we should instead disable tracing and ideally log some record to indicate what happened.

kib added inline comments.May 18 2021, 2:55 PM

sys/kern/vfs_vnops.c
2373 ↗	(On Diff #89383)	This is one approach. But the semantic of SIGXFSZ was defined long time ago. You might equally complain that we kill all debuggees on debugger termination, but this is how FreeBSD kernel behaves. I can easily change this place to not send SIGXFSZ, then ktr_writerequest() would automatically stop tracing anyway, but I am not sure that this is right, even if not desirable.

markj added inline comments.May 18 2021, 3:13 PM

sys/kern/vfs_vnops.c
2373 ↗	(On Diff #89383)	Well, that behaviour of ptrace() is quite annoying, in many cases I believe it is unnecessary. If anything, SIGXFSZ should be sent to the tracing process, not the tracee.

kib marked an inline comment as done.May 18 2021, 4:29 PM

kib added inline comments.

sys/kern/vfs_vnops.c
2373 ↗	(On Diff #89383)	There is no tracing process. Ok. I will add a control, default off, to allow/disallow the signal there.

Add knobs to not send SIGXFSZ on ktrace, and not kill debuggee on debugger exit (not tested).

Per-commit view is available at https://kib.kiev.ua/git/gitweb.cgi?p=deviant3.git;a=shortlog;h=refs/heads/file_limit

Herald added a subscriber: emaste. · View Herald TranscriptMay 18 2021, 4:30 PM

markj added inline comments.May 18 2021, 8:32 PM

sys/kern/kern_ktrace.c
1041 ↗	(On Diff #89433)	This routine bumps the usecount on the vnode, but isn't it already incremented for us?

kib marked an inline comment as done.May 19 2021, 4:25 AM

kib added inline comments.

sys/kern/kern_ktrace.c
1041 ↗	(On Diff #89433)	In fact the issue is that vn_close() was missed.

Drop ptrace changes, I will finish them separately.
Remove excess vref(), and do not forget vn_close() on the tracevp.
Use vrefact() in ktr_writerecord().
Initialize ref count for new ktr_io_params structure to 1, remove increment that is always done.

markj accepted this revision.May 19 2021, 1:57 PM

markj added inline comments.

sys/kern/kern_ktrace.c
153 ↗	(On Diff #89459)
1338 ↗	(On Diff #89459)	Actually I wonder if TDP2_KTRWRITE is really needed, it is guaranteed that TDP_INKTRACE is set here if the write is coming from ktr_writerequest().

This revision is now accepted and ready to land.May 19 2021, 1:57 PM

kib marked 2 inline comments as done.May 19 2021, 2:32 PM

kib added inline comments.

sys/kern/kern_ktrace.c
1338 ↗	(On Diff #89459)	I think, concern there would be for any operation done under ktrace_enter() and resulting in fs write. It seems that it is not possible to do this right now, from code inspection.