Re: [PATCH bpf-next v2 2/2] selftests/bpf: Add test for bpftool access to read-only protected maps

[Slava Imameev <slava.imameev@crowdstrike.com>]

> > +	make -C tools/bpf/bpftool -s -j"$ncpus" OUTPUT="$output_dir"/ >/dev/null
> > +	echo ... finished building bpftool
> > +	cd "$pwd"
> > +}
>
>
> Given that you're reusing the BPF selftests infra, you shouldn't have to
> rebuild bpftool as part of the test. It's already built from the
> Makefile, and other tests just assume it's available already (see
> test_bpftool.py, test_bpftool.sh).


Agree, the build step will be removed for v3.


> > +	# Test write access to the map
> > +	if "$bpftool_path" map update name "$map_name" key $key value $value; then
> > +		if [ "$write_should_succeed" = "true" ]; then
> > +			echo "  Write access to $map_name succeeded as expected"
> > +		else
> > +			echo "  Write access to $map_name succeeded but should have failed"
> > +			exit 1
> > +		fi
> > +	else
> > +		if [ "$write_should_succeed" = "true" ]; then
> > +			echo "  Write access to $map_name failed but should have succeeded"
> > +			exit 1
> > +		else
> > +			echo "  Write access to $map_name failed as expected"
> > +		fi
> > +	fi
> 
> 
> Can we try to delete an item as well, please?

I added an item deletion test to v3.

> > +
> > +	# Pin the map to the BPF filesystem
> > +	"$bpftool_path" map pin name "$map_name" "$pin_path"
> > +	if [ -e "$pin_path" ]; then
> > +		echo "  Successfully pinned $map_name to $pin_path"
> > +	else
> > +		echo "  Failed to pin $map_name"
> > +		exit 1
> > +	fi
> > +
> > +	# Test read access to the pinned map
> > +	if "$bpftool_path" map lookup pinned "$pin_path" key $key; then
> > +		echo "  Read access to pinned $map_name succeeded"
> > +	else
> > +		echo "  Read access to pinned $map_name failed"
> > +		exit 1
> > +	fi
> > +
> > +	# Test write access to the pinned map
> > +	if "$bpftool_path" map update pinned "$pin_path" key $key value $value; then
> > +		if [ "$write_should_succeed" = "true" ]; then
> > +			echo "  Write access to pinned $map_name succeeded as expected"
> > +		else
> > +			echo "  Write access to pinned $map_name succeeded but should have failed"
> > +			exit 1
> > +		fi
> > +	else
> > +		if [ "$write_should_succeed" = "true" ]; then
> > +			echo "  Write access to pinned $map_name failed but should have succeeded"
> > +			exit 1
> > +		else
> > +			echo "  Write access to pinned $map_name failed as expected"
> > +		fi
> > +	fi
> 
> 
> Maybe refactor lookup/update as a function that you can call before and
> after pinning the map? (I don't mind much.)


I changed it as suggested for v3.


> > +check_bpffs() {
> > +	if [ -z "$BPF_FS" ]; then
> > +		echo "Could not run test without bpffs mounted"
> 
> 
> Why not? Bpftool will attempt to mount it for you if it's not available
> (create_and_mount_bpffs_dir()).
> 
> You could mount it manually to a specific location and unmount it during
>  the clean-up phase, if you wanted to be sure that the test doesn't have
> any side effect on the filesystem.


I made changes as suggested for v3.


> > +# Load and attach the BPF programs to control maps access
> > +"$BPFTOOL_PATH" prog loadall "$BPF_FILE_PATH" "$BPF_DIR"/prog autoattach
> > +
> > +# Test protected map (write should fail)
> > +test_map_access "$PROTECTED_MAP_NAME" "false" "$BPFTOOL_PATH" "$BPF_DIR"
> > +
> > +# Test not protected map (write should succeed)
> > +test_map_access "$NOT_PROTECTED_MAP_NAME" "true" "$BPFTOOL_PATH" "$BPF_DIR"
> 
> 
> We could also test map creation here (possibly even with inner maps).

I added a test for map-of-maps creation for v3.