linux-kernel - Re: [PATCH v2 2/3] common/module: add patient module rmmod support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YR0TEE8lUwo6QlHw@bombadil.infradead.org>
Date:   Wed, 18 Aug 2021 07:02:56 -0700
From:   Luis Chamberlain <mcgrof@...nel.org>
To:     Eryu Guan <guan@...u.me>
Cc:     fstests@...r.kernel.org, hare@...e.de, dgilbert@...erlog.com,
        jeyu@...nel.org, lucas.demarchi@...el.com,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/3] common/module: add patient module rmmod support

On Sun, Aug 15, 2021 at 08:29:42PM +0800, Eryu Guan wrote:
> On Wed, Aug 11, 2021 at 08:45:11AM -0700, Luis Chamberlain wrote:
> >  common/config |  31 +++++++++++++++
> >  common/module | 107 ++++++++++++++++++++++++++++++++++++++++++++++++++
> 
> Please also update README to document the new configurable variables.

Got it.

> >  2 files changed, 138 insertions(+)
> > 
> > diff --git a/common/config b/common/config
> > index 005fd50a..9b8a2bc4 100644
> > --- a/common/config
> > +++ b/common/config
> 
> 100s as default seems a bit long to me, use 10s as in v1 patch?

In practice I tried using 10s and from my observations we *still* ran
into false positives. So from my own testing peace of mind I'd prefer at
least something higher, and if its going to be higher might as well go
with something which at least makes painfully safe. I'll go with 50s
for my next submission.

> > +	fi
> > +else
> > +	MODPROBE_RM_PATIENT_TIMEOUT_ARGS=""
> > +	if [[ ! -z "$MODPROBE_PATIENT_RM_TIMEOUT_SECONDS" ]]; then
> > +		if [[ "$MODPROBE_PATIENT_RM_TIMEOUT_MS" != "forever" ]]; then
> 
> Should check MODPROBE_PATIENT_RM_TIMEOUT_SECONDS instead?

Indeed will fix.

> > diff --git a/common/module b/common/module
> > index 39e4e793..03953fa1 100644
> > --- a/common/module
> > +++ b/common/module
> > @@ -4,6 +4,8 @@
> >  #
> >  # Routines for messing around with loadable kernel modules
> >  
> > +source common/config
> > +
> 
> Seems there's no need to source common/config here, as it's sourced in
> common/rc, which is sourced by every test.

OK.

> > +	local max_tries_max=$MODPROBE_PATIENT_RM_TIMEOUT_SECONDS
<-- snip -->

> > +	local max_tries=0
<-- snip -->

> > +	max_tries=$max_tries_max
> > +
> > +	while [[ "$max_tries" != "0" ]]; do
> 
> Use "$max_tries -ne 0" to check inters seems better.

max_tries can be "forever", in which case this is -eq 0:

$ foo="forever"; if [[ $foo -eq 0 ]]; then echo buggy; else echo ok; fi
buggy

> > +			refcnt_is_zero=1
> > +			break
> > +		fi
> > +		sleep 1
> > +		if [[ "$max_tries" == "forever" ]]; then
> > +			continue
> > +		fi
> > +		let max_tries=$max_tries-1
> > +	done
> > +
> > +	if [[ $refcnt_is_zero -ne 1 ]]; then
> > +		echo "custom patient module removal for $module timed out waiting for refcnt to become 0 using timeout of $max_tries_max"
> > +		return -1
> > +	fi
> > +
> > +	# If we ran out of time but our refcnt check confirms we had
> > +	# a refcnt of 0, just try to remove the module once.
> > +	if [[ "$max_tries" == "0" ]]; then
> 
> $max_tries -eq 0

Same issue.

  Luis