On Wed, Apr 09, 2025 at 12:05:11AM +0800, Anand Jain wrote:
>
>
> On 8/4/25 01:22, Darrick J. Wong wrote:
> > On Mon, Apr 07, 2025 at 11:48:18AM +0800, Anand Jain wrote:
> > > Introduce `verify_sysfs_syntax()` and `_require_fs_sysfs_attr_policy()`
> > > to verify whether a sysfs attribute rejects invalid input arguments
> > > during writes.
> > >
> > > Signed-off-by: Anand Jain <anand.jain@xxxxxxxxxx>
> > > ---
> > > common/sysfs | 141 +++++++++++++++++++++++++++++++++++++++++++++++++++
> > > 1 file changed, 141 insertions(+)
> > > create mode 100644 common/sysfs
> > >
> > > diff --git a/common/sysfs b/common/sysfs
> > > new file mode 100644
> > > index 000000000000..9f2d77c6b1f5
> > > --- /dev/null
> > > +++ b/common/sysfs
> > > @@ -0,0 +1,141 @@
> > > +##/bin/bash
> > > +# SPDX-License-Identifier: GPL-2.0+
> > > +# Copyright (c) 2025 Oracle. All Rights Reserved.
> > > +#
> > > +# Common/sysfs file for the sysfs related helper functions.
> > > +
> > > +# Test for the existence of a policy at /sys/fs/$FSTYP/$DEV/$ATTR
> > > +#
> > > +# All arguments are necessary, and in this order:
> > > +# - dev: device name, e.g. $SCRATCH_DEV
> > > +# - attr: path name under /sys/fs/$FSTYP/$dev
> > > +# - policy: policy within /sys/fs/$FSTYP/$dev
> > > +#
> > > +# Usage example:
> > > +# _has_fs_sysfs_attr_policy /dev/mapper/scratch-dev read_policy round-robin
> > > +_has_fs_sysfs_attr_policy()
> > > +{
> > > + local dev=$1
> > > + local attr=$2
> > > + local policy=$3
> > > +
> > > + if [ ! -b "$dev" -o -z "$attr" -o -z "$policy" ]; then
> > > + _fail \
> > > + "Usage: _has_fs_sysfs_attr_policy <mounted_device> <attr> <policy>"
> > > + fi
> >
> > Shouldn't this return 1 if the parameters are not fully specified?
> >
>
> Do you mean, instead of using _fail, just echo "Usage" and return 1?
Doh, I missed that it was _fail and not echo. Please ignore my
comment.
> > > +
> > > + local dname=$(_fs_sysfs_dname $dev)
> > > + test -e /sys/fs/${FSTYP}/${dname}/${attr}
> >
> > What is the point of testing path existence here if the function doesn't
> > actually change its behavior?
> >
>
>
> Ah, I missed the `if`—updated it locally now.
>
> @@ -25,7 +25,9 @@ _has_fs_sysfs_attr_policy()
> fi
>
> local dname=$(_fs_sysfs_dname $dev)
> - test -e /sys/fs/${FSTYP}/${dname}/${attr}
> + if test -e /sys/fs/${FSTYP}/${dname}/${attr}; then
> + return 1
> + fi
<nod>
> cat /sys/fs/${FSTYP}/${dname}/${attr} | grep -q ${policy}
> }
>
>
> > > +
> > > + cat /sys/fs/${FSTYP}/${dname}/${attr} | grep -q ${policy}
> > > +}
> > > +
> > > +# Require the existence of a sysfs entry at /sys/fs/$FSTYP/$DEV/$ATTR
> > > +# and value in it contains $policy
> > > +# All arguments are necessary, and in this order:
> > > +# - dev: device name, e.g. $SCRATCH_DEV
> > > +# - attr: path name under /sys/fs/$FSTYP/$dev
> > > +# - policy: mentioned in /sys/fs/$FSTYP/$dev/$attr
> > > +#
> > > +# Usage example:
> > > +# _require_fs_sysfs_attr_policy /dev/mapper/scratch-dev read_policy round-robin
> > > +_require_fs_sysfs_attr_policy()
> > > +{
> > > + _has_fs_sysfs_attr_policy "$@" && return
> > > +
> > > + local dev=$1
> > > + local attr=$2
> > > + local policy=$3
> > > + local dname=$(_fs_sysfs_dname $dev)
> > > +
> > > + _notrun "This test requires /sys/fs/${FSTYP}/${dname}/${attr} ${policy}"
> > > +}
> > > +
> > > +_set_sysfs_policy()
> > > +{
> > > + local dev=$1
> > > + local attr=$2
> > > + shift
> > > + shift
> > > + local policy=$@
> > > +
> > > + _set_fs_sysfs_attr $dev $attr ${policy}
> > > +
> > > + case "$FSTYP" in
> > > + btrfs)
> > > + _get_fs_sysfs_attr $dev $attr | grep -q "[${policy}]"
> > > + if [[ $? != 0 ]]; then
> > > + echo "Setting sysfs $attr $policy failed"
> > > + fi
> > > + ;;
> > > + *)
> > > + _fail \
> > > +"sysfs syntax verification for '${attr}' '${policy}' for '${FSTYP}' is not implemented"
> > > + ;;
> > > + esac
> > > +}
> > > +
> > > +_set_sysfs_policy_must_fail()
> > > +{
> > > + local dev=$1
> > > + local attr=$2
> > > + shift
> > > + shift
> > > + local policy=$@
> > > +
> > > + _set_fs_sysfs_attr $dev $attr ${policy} | _filter_sysfs_error \
> > > + | tee -a $seqres.full
> > > +}
> > > +
> > > +# Verify sysfs attribute rejects invalid input.
> > > +# Usage syntax:
> > > +# _verify_sysfs_syntax <$dev> <$attr> <$policy> [$value]
> > > +# Examples:
> > > +# _verify_sysfs_syntax $TEST_DEV read_policy pid
> > > +# _verify_sysfs_syntax $TEST_DEV read_policy round-robin 4k
> > > +# Note:
> > > +# Process must call . ./common/filter
> > > +_verify_sysfs_syntax()
> > > +{
> > > + local dev=$1
> > > + local attr=$2
> > > + local policy=$3
> > > + local value=$4
> > > +
> > > + # Do this in the test case so that we know its prerequisites.
> > > + # '_require_fs_sysfs_attr_policy $TEST_DEV $attr $policy'
> >
> > But it's commented out ... ?
>
> Yeah, I commented it out since it's just an example.
> I think I need to rephrase the comment.
Ok.
--D
> Thanks, Anand
>
> >
> > --D
> >
> > > +
> > > + # Test policy specified wrongly. Must fail.
> > > + _set_sysfs_policy_must_fail $dev $attr "'$policy $policy'"
> > > + _set_sysfs_policy_must_fail $dev $attr "'$policy t'"
> > > + _set_sysfs_policy_must_fail $dev $attr "' '"
> > > + _set_sysfs_policy_must_fail $dev $attr "'${policy} n'"
> > > + _set_sysfs_policy_must_fail $dev $attr "'n ${policy}'"
> > > + _set_sysfs_policy_must_fail $dev $attr "' ${policy}'"
> > > + _set_sysfs_policy_must_fail $dev $attr "' ${policy} '"
> > > + _set_sysfs_policy_must_fail $dev $attr "'${policy} '"
> > > + _set_sysfs_policy_must_fail $dev $attr _${policy}
> > > + _set_sysfs_policy_must_fail $dev $attr ${policy}_
> > > + _set_sysfs_policy_must_fail $dev $attr _${policy}_
> > > + _set_sysfs_policy_must_fail $dev $attr ${policy}:
> > > + # Test policy longer than 32 chars fails stable.
> > > + _set_sysfs_policy_must_fail $dev $attr 'jfdkkkkjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjffjfjfffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff'
> > > +
> > > + # Test policy specified correctly. Must pass.
> > > + _set_sysfs_policy $dev $attr $policy
> > > +
> > > + # If the policy has no value return
> > > + if [[ -z $value ]]; then
> > > + return
> > > + fi
> > > +
> > > + # Test value specified wrongly. Must fail.
> > > + _set_sysfs_policy_must_fail $dev $attr "'$policy: $value'"
> > > + _set_sysfs_policy_must_fail $dev $attr "'$policy:$value '"
> > > + _set_sysfs_policy_must_fail $dev $attr "'$policy:$value typo'"
> > > + _set_sysfs_policy_must_fail $dev $attr "'$policy:${value}typo'"
> > > + _set_sysfs_policy_must_fail $dev $attr "'$policy :$value'"
> > > +
> > > + # Test policy and value all specified correctly. Must pass.
> > > + _set_sysfs_policy $dev $attr $policy:$value
> > > +}
> > > --
> > > 2.47.0
> > >
> > >
>
>
