Re: [LTP] [PATCH v3] userfaultfd: Add test using UFFDIO_POISON

[Cyril Hrubis <chrubis@suse.cz>]

Hi!
> > +	SAFE_PTHREAD_JOIN(thr, NULL);
> > +	reset_pages();
> 
> This should also go into a cleanup(), otherwise if other syscalls will
> fail (ioctl for instance), memory will be lost.

Technically all memory is released when last reference to it is removed,
which happens on process exit. So no mmaped() memory is not going to
leak.

> > +	if (poison_fault_seen && sigbus_seen) {
> > +		tst_res(TPASS, "POISON successfully triggered SIGBUS");
> > +	} else if (poison_fault_seen && !sigbus_seen) {
> > +		tst_res(TFAIL, "POISON fault seen but no SIGBUS received");
> > +	} else if (!poison_fault_seen && sigbus_seen) {
> > +		tst_res(TFAIL, "SIGBUS received but no poison fault seen");
> > +	} else {
> > +		tst_res(TFAIL, "No poison fault or SIGBUS observed");
> > +	}
> > +}
> > +
> > +static struct tst_test test = {
> > +	.test_all = run,
> > +	.min_kver = "6.6",
> 
> This is not needed, we should use UFFDIO_API instead, in order to verify
> that UFFD_FEATURE_POISON is present and eventually TCONF.
> 
> 
> Also, if I run checkpatch.pl on the code I get:
> 
> WARNING: Use of volatile is usually wrong: see Documentation/process/volatile-considered-harmful.rst
> #25: FILE: userfaultfd06.c:25:
> +static volatile int poison_fault_seen;

For variables changed from a different thread atomic operation are
better than volatile, i.e. tst_atomic_load() and tst_atomic_store().

> WARNING: Use of volatile is usually wrong: see Documentation/process/volatile-considered-harmful.rst
> #26: FILE: userfaultfd06.c:26:
> +static volatile int sigbus_seen;

This one is wrong, volatile is correct for variables changed for syscall
handlers.

> WARNING: Use of volatile is usually wrong: see Documentation/process/volatile-considered-harmful.rst
> #88: FILE: userfaultfd06.c:88:
> +       volatile char dummy;

I do not think that volatile is needed here.

> WARNING: braces {} are not necessary for any arm of this statement
> #120: FILE: userfaultfd06.c:120:
> +       if (poison_fault_seen && sigbus_seen) {
> [...]
> +       } else if (poison_fault_seen && !sigbus_seen) {
> [...]
> +       } else if (!poison_fault_seen && sigbus_seen) {
> [...]
> +       } else {
> [...]
> 
> total: 0 errors, 4 warnings, 134 lines checked
> 
> NOTE: For some of the reported defects, checkpatch may be able to
>       mechanically convert to the typical style using --fix or --fix-inplace.
> 
> userfaultfd06.c has style problems, please review.
> 
> NOTE: If any of the errors are false positives, please report
>       them to the maintainer, see CHECKPATCH in MAINTAINERS.
> 
> If you have `b4`, please run `b4 prep --check` before sending the patch.
> 
> -- 
> Andrea Cervesato
> SUSE QE Automation Engineer Linux
> andrea.cervesato@suse.com
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp