xfsqa: test open_by_handle() on unlinked and freed inode clusters V2
PATCH 1/1 xfsqa: test open_by_handle() on unlinked and freed inode clusters V2
From: Dave Chinner <[email protected]>
When Christoph and I were discussing bulkstat coherency on IRC, we realised that inode lookup from bulkstat was not actually looking up the inode allocation btree in xfs_imap() before reading the inode buffer from disk in xfs_iread(). Bulkstat uses the same lookup mechanism as handle validation to avoid shutting down the filesystem if inode numbers that point to non-inode buffers (i.e. invalid) are passed in the handle.
The problem with this is that when we delete inodes from disk and we remove the inode chunk (i.e. deallocate inodes) we mark both the inodes in memory and the cluster buffer as stale, thereby preventing it from being written back to disk. The result of this is that some number of inodes remain on disk looking like allocated, in use inodes (i.e. di_mode is not zero).
Hence if we get a cold cache lookup from a stale handle that references such an inode, we can read the inode off disk even though it has been deleted because we don't check if the inode is allocated or not. If the inode chunk has not been overwritten, then the inode read will succeed and the handle-to-dentry conversion will not error out like it is supposed to. The result is that stale NFS filehandles and open_by_handle() will succeed incorrectly on unlinked files for cold cache lookups.
This is a bug that has been present ever since the inode chunk deletion code was implemented. This test exercises the problem and documents the hoops you have to jump through to reproduce it.
Version 2:
o suppress mkfs output that was unnoticed due to failure noise
o fix exit value so test reports success correctly (e.g. when run with MOUNT_OPTIONS="-o ikeep")
--- 238 | 57 +++++++++++++++++++++ 238.out | 2 + group | 1 + src/Makefile | 3 +- src/stale_handle.c | 140 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 202 insertions(+), 1 deletions(-) create mode 100755 238 create mode 100644 238.out create mode 100644 src/stale_handle.c
238 new file +57 -0
@@ -0,0 +1,57 @@ +#! /bin/bash +# FS QA Test No. 238 +# +# Check stale handles pointing to unlinked files are detected correctly +# +#----------------------------------------------------------------------- +# Copyright (c) 2010 Red Hat, Inc. All Rights Reserved. +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License as +# published by the Free Software Foundation. +# +# This program is distributed in the hope that it would be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write the Free Software Foundation, +# Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA +#----------------------------------------------------------------------- +# +# creator [email protected] + +seq=`basename $0` +echo "QA output created by $seq" + +here=`pwd` +tmp=/tmp/$$ +status=1 # failure is the default! +trap "_cleanup; exit \$status" 0 1 2 3 15 + +_cleanup() +{ + cd / + rm -f $tmp.* +} + +# get standard environment, filters and checks +. ./common.rc +. ./common.filter + +# real QA test starts here + +# Modify as appropriate. +_supported_fs xfs +_supported_os Linux +_require_scratch + +echo "Silence is golden" + +_scratch_mkfs > /dev/null 2>&1 +_scratch_mount > /dev/null 2>&1 +src/stale_handle $SCRATCH_MNT +status=$? +exit
238.out new file +2 -0
@@ -0,0 +1,2 @@ +QA output created by 238 +Silence is golden
group +1 -0
@@ -351,3 +351,4 @@ deprecated 235 auto quota quick 236 auto quick metadata 237 auto quick acl +238 auto quick metadata ioctl
src/Makefile +2 -1
@@ -15,7 +15,8 @@ TARGETS = dirstress fill fill2 getpagesize holes lstat64 \ LINUX_TARGETS = xfsctl bstat t_mtab getdevicesize preallo_rw_pattern_reader \ preallo_rw_pattern_writer ftrunc trunc fs_perms testx looptest \ locktest unwritten_mmap bulkstat_unlink_test t_stripealign \ - bulkstat_unlink_test_modified t_dir_offset t_futimens t_immutable + bulkstat_unlink_test_modified t_dir_offset t_futimens t_immutable \ + stale_handle SUBDIRS =
src/stale_handle.c new file +140 -0
140 lines not on this page · raw message
Signature · 6 lines
1.7.1 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Quoted 8 lines · On Donnerstag, 17. Juni 2010 Dave Chinner wrote:
Hence if we get a cold cache lookup from a stale handle that references such an inode, we can read the inode off disk even though it has been deleted because we don't check if the inode is allocated or not. If the inode chunk has not been overwritten, then the inode read will succeed and the handle-to-dentry conversion will not error out like it is supposed to. The result is that stale NFS filehandles and
open_by_handle()will succeed incorrectly on unlinked files for cold cache lookups.
Wouldn't that qualify as a security problem and be handled as such? There should be back ports for "long term support" kernels of security- sensitive people, and so on.
Signature · 10 lines
mit freundlichen Grüssen, Michael Monnerie, Ing. BSc it-management Internet Services http://proteger.at [gesprochen: Prot-e-schee] Tel: 0660 / 415 65 31 // Wir haben im Moment zwei Häuser zu verkaufen: // http://zmi.at/langegg/ // http://zmi.at/haus2009/
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 10 lines · Michael Monnerie
Quoted 8 lines · On Donnerstag, 17. Juni 2010 Dave Chinner wrote:
Hence if we get a cold cache lookup from a stale handle that references such an inode, we can read the inode off disk even though it has been deleted because we don't check if the inode is allocated or not. If the inode chunk has not been overwritten, then the inode read will succeed and the handle-to-dentry conversion will not error out like it is supposed to. The result is that stale NFS filehandles and
open_by_handle()will succeed incorrectly on unlinked files for cold cache lookups.
Quoted 3 lines
Wouldn't that qualify as a security problem and be handled as such? There should be back ports for "long term support" kernels of security- sensitive people, and so on.
Probably. Alex, are you able to handle this side of things?
Note that local open_by_handle() use is not really an issue - it requires root and if you have root you can run xfs_db or dd on the block device to get the same information.
Cheers,
Dave.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Looks good,
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs