* Re: [PATCH 7/8] md: skip resync for raid array with journal
From: Neil Brown @ 2015-09-30 4:24 UTC (permalink / raw)
To: Shaohua Li, linux-raid; +Cc: Kernel-team, songliubraving, hch, dan.j.williams
In-Reply-To: <7c5622ab3335f31a27f807d8fee27f8bb60675ae.1441221530.git.shli@fb.com>
[-- Attachment #1: Type: text/plain, Size: 2608 bytes --]
Shaohua Li <shli@fb.com> writes:
> If a raid array has journal, the journal can guarantee the consistency,
> we can skip resync after a unclean shutdown. The exception is raid
> creation or user initiated resync, which we still do a raid resync.
>
> Signed-off-by: Shaohua Li <shli@fb.com>
> ---
> drivers/md/md.c | 4 ++++
> drivers/md/md.h | 1 +
> 2 files changed, 5 insertions(+)
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index b3f9eed..95824fb 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -1669,6 +1669,8 @@ static int super_1_validate(struct mddev *mddev, struct md_rdev *rdev)
> }
> set_bit(Journal, &rdev->flags);
> rdev->journal_tail = le64_to_cpu(sb->journal_tail);
> + if (mddev->recovery_cp == MaxSector)
> + set_bit(MD_JOURNAL_CLEAN, &mddev->flags);
> break;
> default:
> rdev->saved_raid_disk = role;
> @@ -1711,6 +1713,8 @@ static void super_1_sync(struct mddev *mddev, struct md_rdev *rdev)
> sb->events = cpu_to_le64(mddev->events);
> if (mddev->in_sync)
> sb->resync_offset = cpu_to_le64(mddev->recovery_cp);
> + else if (test_bit(MD_JOURNAL_CLEAN, &mddev->flags))
> + sb->resync_offset = cpu_to_le64(MaxSector);
> else
> sb->resync_offset = cpu_to_le64(0);
>
> diff --git a/drivers/md/md.h b/drivers/md/md.h
> index 226f4ba..0288a0b 100644
> --- a/drivers/md/md.h
> +++ b/drivers/md/md.h
> @@ -236,6 +236,7 @@ struct mddev {
> #define MD_STILL_CLOSED 4 /* If set, then array has not been opened since
> * md_ioctl checked on it.
> */
> +#define MD_JOURNAL_CLEAN 5 /* A raid with journal is already clean */
>
> int suspended;
> atomic_t active_io;
> --
> 1.8.1
This looks right as far as it goes, but I don't think it goes far
enough.
The particular scenario that bothers me is if the array is started
without the journal being present.
I cannot see anything to prevent that - is there?
In that case we need to assume that the array is not in-sync,
and we need to clear MD_FEATURE_JOURNAL so if it gets stopped and then
assembled again with the stale journal doesn't get used.
One unfortunate side effect of that is that you couldn't stop the array
cleanly (leaving the journal effectively empty) and then restart with no
journal and no resync. Is that a problem I wonder?
I'm not sure what the best solution is here, but we need a clear
understanding of what happens if you try to assemble an array without
the journal where previously it had one, and I don't think the current
code gets it right.
Thanks,
NeilBrown
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]
^ permalink raw reply
* Re: [PATCH 0/8] raid5-cache fixes
From: Neil Brown @ 2015-09-30 5:36 UTC (permalink / raw)
To: Shaohua Li, linux-raid; +Cc: Kernel-team, songliubraving, hch, dan.j.williams
In-Reply-To: <cover.1441221530.git.shli@fb.com>
[-- Attachment #1: Type: text/plain, Size: 1303 bytes --]
Shaohua Li <shli@fb.com> writes:
> Fix some bugs, improve performance and add trim/resync support.
>
> Thanks,
> Shaohua
>
> Christoph Hellwig (1):
> raid5-cache: move functionality out of __r5l_set_io_unit_state
>
> Shaohua Li (7):
> md: fix feature map check
> raid5: fix build error
> raid5-cache: switching to state machine for log disk cache flush
> raid5-cache: fix a user-after-free bug
> raid5-cache: optimize FLUSH IO with log enabled
> md: skip resync for raid array with journal
> raid5-cache: add trim support for log
>
> drivers/md/md.c | 6 +-
> drivers/md/md.h | 1 +
> drivers/md/raid5-cache.c | 254 ++++++++++++++++++++++++++++-------------------
> drivers/md/raid5.c | 17 +++-
> drivers/md/raid5.h | 1 +
> 5 files changed, 175 insertions(+), 104 deletions(-)
>
> --
> 1.8.1
Thanks.
I have applied all except
> raid5-cache: add trim support for log
which I have replied to separately.
I have also applied the other two patches you sent separately.
I've made a number of white-space fixes throughout the series,
and I've merged Christoph's "port to 4.3-rc" patch into various places
in the series.
This is all in the 'devel' branch of git://neil.brown.name/md/
Thanks,
NeilBrown
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]
^ permalink raw reply
* Re: raid5-cache I/O path improvements V2
From: Neil Brown @ 2015-09-30 5:39 UTC (permalink / raw)
To: Shaohua Li
Cc: Christoph Hellwig, linux-raid@vger.kernel.org, Kernel Team,
dan.j.williams@intel.com
In-Reply-To: <20150928140127.GA30927@lst.de>
[-- Attachment #1: Type: text/plain, Size: 662 bytes --]
Christoph Hellwig <hch@lst.de> writes:
> So the summary is that for now you want me to resend with a patch
> to opt into using FUA?
I'd like to avoid "opt in" if at all possible.
Shoahua measured that using "FUA" for all writes to the journal
hurt performance on at least one device. Do you have a different device
where it demonstrably helps?
If there any chance of automatically detecting which is which?
>
> Also do you have a git tree as a baseline somewhere? I've been collecting
> the patches you've sent, but with increasing time I fear I might have
> missed something.
(almost) all at git://neil.brown.name/md in 'devel' branch.
Thanks,
NeilBrown
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]
^ permalink raw reply
* [md:devel 19/30] drivers/md/raid5-cache.c:836:24: sparse: incompatible types in comparison expression (different address spaces)
From: kbuild test robot @ 2015-09-30 6:07 UTC (permalink / raw)
To: Shaohua Li; +Cc: kbuild-all, NeilBrown, linux-raid
tree: git://neil.brown.name/md devel
head: c086427458276f6265b609a682651786e1281c82
commit: a62ddcf73e2fe3387dbf27280691045446be0e15 [19/30] raid5: log recovery
reproduce:
# apt-get install sparse
git checkout a62ddcf73e2fe3387dbf27280691045446be0e15
make ARCH=x86_64 allmodconfig
make C=1 CF=-D__CHECK_ENDIAN__
sparse warnings: (new ones prefixed by >>)
>> drivers/md/raid5-cache.c:836:24: sparse: incompatible types in comparison expression (different address spaces)
drivers/md/raid5-cache.c:840:25: sparse: incompatible types in comparison expression (different address spaces)
vim +836 drivers/md/raid5-cache.c
820 continue;
821 addr = kmap_atomic(sh->dev[disk_index].page);
822 checksum = crc32_le(log->uuid_checksum, addr, PAGE_SIZE);
823 kunmap_atomic(addr);
824 if (checksum != sh->dev[disk_index].log_checksum)
825 goto error;
826 }
827
828 for (disk_index = 0; disk_index < sh->disks; disk_index++) {
829 struct md_rdev *rdev, *rrdev;
830
831 if (!test_and_clear_bit(R5_Wantwrite,
832 &sh->dev[disk_index].flags))
833 continue;
834
835 /* in case device is broken */
> 836 rdev = rcu_dereference(conf->disks[disk_index].rdev);
837 if (rdev)
838 sync_page_io(rdev, stripe_sect, PAGE_SIZE,
839 sh->dev[disk_index].page, WRITE, false);
840 rrdev = rcu_dereference(conf->disks[disk_index].replacement);
841 if (rrdev)
842 sync_page_io(rrdev, stripe_sect, PAGE_SIZE,
843 sh->dev[disk_index].page, WRITE, false);
844 }
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
^ permalink raw reply
* Re: [PATCH 1/2] md: clear CHANGE_PENDING in readonly array
From: Neil Brown @ 2015-09-30 6:59 UTC (permalink / raw)
To: Shaohua Li; +Cc: linux-raid, Kernel-team, songliubraving, hch, dan.j.williams
In-Reply-To: <20150924164720.GA3706482@devbig084.prn1.facebook.com>
[-- Attachment #1: Type: text/plain, Size: 3316 bytes --]
Shaohua Li <shli@fb.com> writes:
> On Thu, Sep 24, 2015 at 02:03:53PM +1000, Neil Brown wrote:
>> Shaohua Li <shli@fb.com> writes:
>>
>> > On Wed, Sep 23, 2015 at 04:05:33PM +1000, Neil Brown wrote:
>> >> Shaohua Li <shli@fb.com> writes:
>> >>
>> >> > If faulty disks of an array are more than allowed degraded number, the
>> >> > array enters error handling. It will be marked as read-only with
>> >> > MD_CHANGE_PENDING/RECOVERY_NEEDED set. But currently recovery doesn't
>> >> > clear CHANGE_PENDING bit for read-only array. If MD_CHANGE_PENDING is
>> >> > set for a raid5 array, all returned IO will be hold on a list till the
>> >> > bit is clear. But recovery nevery clears this bit, the IO is always in
>> >> > pending state and nevery finish. This has bad effects like upper layer
>> >> > can't get an IO error and the array can't be stopped.
>> >> >
>> >> > Signed-off-by: Shaohua Li <shli@fb.com>
>> >> > ---
>> >> > drivers/md/md.c | 1 +
>> >> > 1 file changed, 1 insertion(+)
>> >> >
>> >> > diff --git a/drivers/md/md.c b/drivers/md/md.c
>> >> > index 95824fb..c596b73 100644
>> >> > --- a/drivers/md/md.c
>> >> > +++ b/drivers/md/md.c
>> >> > @@ -8209,6 +8209,7 @@ void md_check_recovery(struct mddev *mddev)
>> >> > md_reap_sync_thread(mddev);
>> >> > clear_bit(MD_RECOVERY_RECOVER, &mddev->recovery);
>> >> > clear_bit(MD_RECOVERY_NEEDED, &mddev->recovery);
>> >> > + clear_bit(MD_CHANGE_PENDING, &mddev->flags);
>> >> > goto unlock;
>> >> > }
>> >> >
>> >> > --
>> >> > 1.8.1
>> >>
>> >> Hi,
>> >> I can see that clearing MD_CHANGE_PENDING there is probably correct -
>> >> bug introduced by
>> >> Commit: c3cce6cda162 ("md/raid5: ensure device failure recorded before write request returns.")
>> >>
>> >> However I don't understand your reasoning. You say that the array is
>> >> marked as read-only, but I don't see how that would happen. What
>> >> causes the array to be marked "read-only"?
>> >
>> > It's set read-only by mdadm. I didn't look carefully, but looks there is
>> > disk failure event, mdadm is invoked automatically by some background
>> > daemon. It's a ubuntu distribution.
>>
>> Thanks.
>> This raises a couple of questions.
>>
>> 1/ What should md_set_readonly do if it finds that MD_CHANGE_PENDING is
>> set?
>> Maybe it should wait for md_check_recovery to get run which should
>> clear the bit, after probably writing out the metadata.
>>
>> 2/ Why didn't md_check_recovery already do that before mdadm had a
>> chance to set the array read-only?
>> I guess that is just a timing thing. md_check_recovery could be
>> delayed, and mdadm could get called by udev rather quickly.
>>
>> I think I'll get md_set_readonly to
>> wait_event(mddev->sb_wait,
>> !test_bit(MD_CHANGE_PENDING, &mddev->flags));
>>
>> because I think that is the right thing to do. But if the array is
>> already read-only that won't help, so I'll still need you patch.
>>
>> Would you be able to test that the following patch (without your patch)
>> also fixes the symptom?
>
> Yes, the wait_event patch fixes the issue (without my patch).
>
> Thanks,
> Shaohua
Thanks for testing. I change "Reported-by:" to
"Reported-and-tested-by:" :-)
NeilBrown
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]
^ permalink raw reply
* Re: [PATCH] enable bypass raid5 journal for full stripe writes
From: Neil Brown @ 2015-09-30 8:03 UTC (permalink / raw)
To: linux-raid; +Cc: shli, hch, dan.j.williams, hch, Kernel-team, Song Liu
In-Reply-To: <1441346487-4011056-1-git-send-email-songliubraving@fb.com>
[-- Attachment #1: Type: text/plain, Size: 5895 bytes --]
Song Liu <songliubraving@fb.com> writes:
> Summary:
> To save life time of journal device, we can config the device to
> bypass journal writes for full stripe write. This is configured by:
>
> echo "yes" > /sys/block/mdX/md/r5l_bypass_full_stripe
>
> and
>
> echo "no" > /sys/block/mdX/md/r5l_bypass_full_stripe
>
> For file system integrity, full stripe with REQ_FUA will still
> write to journal first.
>
> This patch applies on top of Shaohua's most recent patches:
>
> http://marc.info/?l=linux-raid&m=144122700510667
This just re-introduces the write hole.
You have no guarantee that all of the writes will complete before a
crash, but some might.
So after a crash on a degraded array, you still get unreliable data.
NeilBrown
>
> Signed-off-by: Song Liu <songliubraving@fb.com>
> Reviewed-by: Shaohua Li <shli@fb.com>
> ---
> drivers/md/raid5-cache.c | 20 +++++++++++++++++
> drivers/md/raid5.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++
> drivers/md/raid5.h | 2 ++
> 3 files changed, 78 insertions(+)
>
> diff --git a/drivers/md/raid5-cache.c b/drivers/md/raid5-cache.c
> index 410b85b..0c3ddc5 100644
> --- a/drivers/md/raid5-cache.c
> +++ b/drivers/md/raid5-cache.c
> @@ -82,6 +82,8 @@ struct r5l_log {
>
> struct list_head no_space_stripes; /* pending stripes, log has no space */
> spinlock_t no_space_stripes_lock;
> +
> + int bypass_full_stripe;
> };
>
> /*
> @@ -438,6 +440,7 @@ int r5l_write_stripe(struct r5l_log *log, struct stripe_head *sh)
> int meta_size;
> int reserve;
> int i;
> + int fua = 0;
>
> if (!log)
> return -EAGAIN;
> @@ -453,6 +456,8 @@ int r5l_write_stripe(struct r5l_log *log, struct stripe_head *sh)
> void *addr;
> if (!test_bit(R5_Wantwrite, &sh->dev[i].flags))
> continue;
> + if (test_bit(R5_WantFUA, &sh->dev[i].flags))
> + fua = 1;
> write_disks++;
> /* checksum is already calculated in last run */
> if (test_bit(STRIPE_LOG_TRAPPED, &sh->state))
> @@ -462,6 +467,10 @@ int r5l_write_stripe(struct r5l_log *log, struct stripe_head *sh)
> addr, PAGE_SIZE);
> kunmap_atomic(addr);
> }
> +
> + if (log->bypass_full_stripe && (write_disks == sh->disks) && (!fua))
> + return -EAGAIN; /* bypass journal device */
> +
> parity_pages = 1 + !!(sh->qd_idx >= 0);
> data_pages = write_disks - parity_pages;
>
> @@ -520,6 +529,16 @@ int r5l_handle_flush_request(struct r5l_log *log, struct bio *bio)
> return -EAGAIN;
> }
>
> +int r5l_get_bypass_full_stripe(struct r5l_log *log)
> +{
> + return log->bypass_full_stripe;
> +}
> +
> +void r5l_set_bypass_full_stripe(struct r5l_log *log, int val)
> +{
> + log->bypass_full_stripe = val;
> +}
> +
> /* This will run after log space is reclaimed */
> static void r5l_run_no_space_stripes(struct r5l_log *log)
> {
> @@ -1105,6 +1124,7 @@ int r5l_init_log(struct r5conf *conf, struct md_rdev *rdev)
> if (!log->io_kc)
> goto io_kc;
>
> + log->bypass_full_stripe = 0;
> log->reclaim_thread = md_register_thread(r5l_reclaim_thread,
> log->rdev->mddev, "reclaim");
> if (!log->reclaim_thread)
> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index 394cdf8..5781987 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -6223,6 +6223,61 @@ raid5_group_thread_cnt = __ATTR(group_thread_cnt, S_IRUGO | S_IWUSR,
> raid5_show_group_thread_cnt,
> raid5_store_group_thread_cnt);
>
> +static ssize_t
> +r5l_show_bypass_full_stripe(struct mddev *mddev, char *page)
> +{
> + struct r5conf *conf;
> + int ret = 0;
> +
> + spin_lock(&mddev->lock);
> + conf = mddev->private;
> + if (conf) {
> + if (conf->log)
> + ret = sprintf(page, "%s\n",
> + r5l_get_bypass_full_stripe(conf->log) ? "yes" : "no");
> + else
> + ret = sprintf(page, "n/a\n");
> + }
> + spin_unlock(&mddev->lock);
> + return ret;
> +}
> +
> +static ssize_t
> +r5l_store_bypass_full_stripe(struct mddev *mddev, const char *page, size_t len)
> +{
> + struct r5conf *conf;
> + int err = 0;
> + int val;
> +
> + if (strncmp(page, "yes", 3) == 0 &&
> + (page[3] == '\n' || page[3] == '\0'))
> + val = 1;
> + else if (strncmp(page, "no", 2) == 0 &&
> + (page[2] == '\n' || page[2] == '\0'))
> + val = 0;
> + else
> + return -EINVAL;
> +
> + mddev_suspend(mddev);
> + spin_lock(&mddev->lock);
> + conf = mddev->private;
> + if (conf) {
> + if (conf->log) {
> + r5l_set_bypass_full_stripe(conf->log, val);
> + } else
> + err = -EINVAL;
> + } else
> + err = -ENODEV;
> + spin_unlock(&mddev->lock);
> + mddev_resume(mddev);
> + return err ?: len;
> +}
> +
> +static struct md_sysfs_entry
> +r5l_bypass_full_stripe = __ATTR(r5l_bypass_full_stripe, S_IRUGO | S_IWUSR,
> + r5l_show_bypass_full_stripe,
> + r5l_store_bypass_full_stripe);
> +
> static struct attribute *raid5_attrs[] = {
> &raid5_stripecache_size.attr,
> &raid5_stripecache_active.attr,
> @@ -6230,6 +6285,7 @@ static struct attribute *raid5_attrs[] = {
> &raid5_group_thread_cnt.attr,
> &raid5_skip_copy.attr,
> &raid5_rmw_level.attr,
> + &r5l_bypass_full_stripe.attr,
> NULL,
> };
> static struct attribute_group raid5_attrs_group = {
> diff --git a/drivers/md/raid5.h b/drivers/md/raid5.h
> index e6b9a40..c1f6935 100644
> --- a/drivers/md/raid5.h
> +++ b/drivers/md/raid5.h
> @@ -630,4 +630,6 @@ extern void r5l_write_stripe_run(struct r5l_log *log);
> extern void r5l_flush_stripe_to_raid(struct r5l_log *log);
> extern void r5l_stripe_write_finished(struct stripe_head *sh);
> extern int r5l_handle_flush_request(struct r5l_log *log, struct bio *bio);
> +extern int r5l_get_bypass_full_stripe(struct r5l_log *log);
> +extern void r5l_set_bypass_full_stripe(struct r5l_log *log, int val);
> #endif
> --
> 1.8.1
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]
^ permalink raw reply
* Re: [PATCH] skip match_mddev_units check for special roles
From: Neil Brown @ 2015-09-30 8:06 UTC (permalink / raw)
To: linux-raid; +Cc: shli, hch, dan.j.williams, hch, Kernel-team, Song Liu
In-Reply-To: <1441346435-4009054-1-git-send-email-songliubraving@fb.com>
[-- Attachment #1: Type: text/plain, Size: 1659 bytes --]
Song Liu <songliubraving@fb.com> writes:
> match_mddev_units is used to check whether 2 RAID arrays share
> same disk(s). Arrays that share disk(s) will not do resync at the
> same time for better performance (fewer HDD seek). However, this
> check should not apply to Spare, Faulty, and Journal disks, as
> they do not paticipate in resync.
>
> In this patch, match_mddev_units skips check for disks with flag
> "Faulty" or "Journal" or raid_disk < 0.
>
> Signed-off-by: Song Liu <songliubraving@fb.com>
> Signed-off-by: Shaohua Li <shli@fb.com>
> ---
> drivers/md/md.c | 14 ++++++++++++--
> 1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 95824fb..b96d135 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -1957,13 +1957,23 @@ static int match_mddev_units(struct mddev *mddev1, struct mddev *mddev2)
> struct md_rdev *rdev, *rdev2;
>
> rcu_read_lock();
> - rdev_for_each_rcu(rdev, mddev1)
> - rdev_for_each_rcu(rdev2, mddev2)
> + rdev_for_each_rcu(rdev, mddev1) {
> + if (test_bit(Faulty, &rdev->flags) ||
> + test_bit(Journal, &rdev->flags) ||
> + rdev->raid_disk == -1)
> + continue;
> + rdev_for_each_rcu(rdev2, mddev2) {
> + if (test_bit(Faulty, &rdev2->flags) ||
> + test_bit(Journal, &rdev2->flags) ||
> + rdev2->raid_disk == -1)
> + continue;
> if (rdev->bdev->bd_contains ==
> rdev2->bdev->bd_contains) {
> rcu_read_unlock();
> return 1;
> }
> + }
> + }
> rcu_read_unlock();
> return 0;
> }
> --
> 1.8.1
That makes sense. Applied, thanks.
NeilBrown
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]
^ permalink raw reply
* RE:
From: Apple-Free-Lotto @ 2015-09-30 12:06 UTC (permalink / raw)
To: Recipients
You have won 760,889:00 GBP in Apple Free Lotto, without the sale of any tickets! Send. Full Name:. Mobile Number and Alternative Email Address. for details and instructions please contact Mr. Gilly Mann: Email: app.freeloto@foxmail.com
^ permalink raw reply
* Re: raid5-cache I/O path improvements V2
From: Christoph Hellwig @ 2015-09-30 15:00 UTC (permalink / raw)
To: Neil Brown
Cc: Christoph Hellwig, Shaohua Li, linux-raid@vger.kernel.org,
Kernel Team, dan.j.williams@intel.com
In-Reply-To: <87vbaslb2v.fsf@notabene.neil.brown.name>
On Wed, Sep 30, 2015 at 03:39:52PM +1000, Neil Brown wrote:
> Christoph Hellwig <hch@lst.de> writes:
>
> > So the summary is that for now you want me to resend with a patch
> > to opt into using FUA?
>
> I'd like to avoid "opt in" if at all possible.
> Shoahua measured that using "FUA" for all writes to the journal
> hurt performance on at least one device. Do you have a different device
> where it demonstrably helps?
> If there any chance of automatically detecting which is which?
I have a high end SAS SSD where it helps, but the real use case where
it makes a major difference are battery backed dimms (NV-DIMMS) or
other devices where we don't even need the FUA bit as they don't have
a cache at all. The important part is to avoid the batching up for
the non-existant flush in that case.
So I could defintively default the code to on only for those, but not
even allowing a tunable for devices that have the FUA bit seems like
an odd restriction.
^ permalink raw reply
* [PATCH 0/5] User namespace mount updates
From: Seth Forshee @ 2015-09-30 20:15 UTC (permalink / raw)
To: Eric W. Biederman, linux-bcache, dm-devel, linux-raid, linux-mtd,
linux-fsdevel, linux-security-module, selinux
Cc: Alexander Viro, Serge Hallyn, Andy Lutomirski, linux-kernel,
Seth Forshee
Hi Eric,
Here's a batch of updates for the unprivileged user namespace mount
patches based on your feedback. I think everything you mentioned should
be addressed here.
These are now based on your for-testing branch.
Updates include:
- Fix for incorrect use of flags argument in mount_mtd.
- Eliminate lookup_bdev_perm and instead add an access mode argument to
lookup_bdev.
- Use __inode_permission instead of inode_permission when checking for
rights towards a block device inode.
- Add a patch replacing in_user_ns with current_in_user_ns.
- Add a patch to handle Smack security labels consistently.
Thanks,
Seth
Andy Lutomirski (1):
fs: Treat foreign mounts as nosuid
Seth Forshee (4):
fs: Verify access of user towards block device file when mounting
selinux: Add support for unprivileged mounts from user namespaces
userns: Replace in_userns with current_in_userns
Smack: Handle labels consistently in untrusted mounts
drivers/md/bcache/super.c | 2 +-
drivers/md/dm-table.c | 2 +-
drivers/mtd/mtdsuper.c | 6 +++++-
fs/block_dev.c | 18 +++++++++++++++---
fs/exec.c | 2 +-
fs/namespace.c | 13 +++++++++++++
fs/quota/quota.c | 2 +-
include/linux/fs.h | 2 +-
include/linux/mount.h | 1 +
include/linux/user_namespace.h | 6 ++----
kernel/user_namespace.c | 6 +++---
security/commoncap.c | 4 ++--
security/selinux/hooks.c | 25 ++++++++++++++++++++++++-
security/smack/smack_lsm.c | 28 ++++++++++++++++++----------
14 files changed, 88 insertions(+), 29 deletions(-)
^ permalink raw reply
* [PATCH 1/5] fs: Verify access of user towards block device file when mounting
From: Seth Forshee @ 2015-09-30 20:15 UTC (permalink / raw)
To: Eric W. Biederman, Kent Overstreet, Alasdair Kergon, Mike Snitzer,
dm-devel, Neil Brown, David Woodhouse, Brian Norris,
Alexander Viro, Jan Kara, Jeff Layton, J. Bruce Fields
Cc: Serge Hallyn, Andy Lutomirski, linux-fsdevel,
linux-security-module, selinux, linux-kernel, linux-mtd,
linux-bcache, linux-raid, Seth Forshee
In-Reply-To: <1443644116-41366-1-git-send-email-seth.forshee@canonical.com>
When mounting a filesystem on a block device there is currently
no verification that the user has appropriate access to the
device file passed to mount. This has not been an issue so far
since the user in question has always been root, but this must
be changed before allowing unprivileged users to mount in user
namespaces.
To fix this, add an argument to lookup_bdev() to specify the
required permissions. If the mask of permissions is zero, or
if the user has CAP_SYS_ADMIN, the permission check is skipped,
otherwise the lookup fails if the user does not have the
specified access rights for the inode at the supplied path.
Callers associated with mounting are updated to pass permission
masks to lookup_bdev() so that these mounts will fail for an
unprivileged user who lacks permissions for the block device
inode. All other callers pass 0 to maintain their current
behaviors.
Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
---
drivers/md/bcache/super.c | 2 +-
drivers/md/dm-table.c | 2 +-
drivers/mtd/mtdsuper.c | 6 +++++-
fs/block_dev.c | 18 +++++++++++++++---
fs/quota/quota.c | 2 +-
include/linux/fs.h | 2 +-
6 files changed, 24 insertions(+), 8 deletions(-)
diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
index 679a093a3bf6..e8287b0d1dac 100644
--- a/drivers/md/bcache/super.c
+++ b/drivers/md/bcache/super.c
@@ -1926,7 +1926,7 @@ static ssize_t register_bcache(struct kobject *k, struct kobj_attribute *attr,
sb);
if (IS_ERR(bdev)) {
if (bdev == ERR_PTR(-EBUSY)) {
- bdev = lookup_bdev(strim(path));
+ bdev = lookup_bdev(strim(path), 0);
mutex_lock(&bch_register_lock);
if (!IS_ERR(bdev) && bch_is_open(bdev))
err = "device already registered";
diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
index e76ed003769e..35bb3ea4cbe2 100644
--- a/drivers/md/dm-table.c
+++ b/drivers/md/dm-table.c
@@ -380,7 +380,7 @@ int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode,
BUG_ON(!t);
/* convert the path to a device */
- bdev = lookup_bdev(path);
+ bdev = lookup_bdev(path, 0);
if (IS_ERR(bdev)) {
dev = name_to_dev_t(path);
if (!dev)
diff --git a/drivers/mtd/mtdsuper.c b/drivers/mtd/mtdsuper.c
index 20c02a3b7417..5d7e7705fed8 100644
--- a/drivers/mtd/mtdsuper.c
+++ b/drivers/mtd/mtdsuper.c
@@ -125,6 +125,7 @@ struct dentry *mount_mtd(struct file_system_type *fs_type, int flags,
#ifdef CONFIG_BLOCK
struct block_device *bdev;
int ret, major;
+ int perm;
#endif
int mtdnr;
@@ -176,7 +177,10 @@ struct dentry *mount_mtd(struct file_system_type *fs_type, int flags,
/* try the old way - the hack where we allowed users to mount
* /dev/mtdblock$(n) but didn't actually _use_ the blockdev
*/
- bdev = lookup_bdev(dev_name);
+ perm = MAY_READ;
+ if (!(flags & MS_RDONLY))
+ perm |= MAY_WRITE;
+ bdev = lookup_bdev(dev_name, perm);
if (IS_ERR(bdev)) {
ret = PTR_ERR(bdev);
pr_debug("MTDSB: lookup_bdev() returned %d\n", ret);
diff --git a/fs/block_dev.c b/fs/block_dev.c
index 26cee058dc02..54d94cd64577 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -1394,9 +1394,14 @@ struct block_device *blkdev_get_by_path(const char *path, fmode_t mode,
void *holder)
{
struct block_device *bdev;
+ int perm = 0;
int err;
- bdev = lookup_bdev(path);
+ if (mode & FMODE_READ)
+ perm |= MAY_READ;
+ if (mode & FMODE_WRITE)
+ perm |= MAY_WRITE;
+ bdev = lookup_bdev(path, perm);
if (IS_ERR(bdev))
return bdev;
@@ -1706,12 +1711,14 @@ EXPORT_SYMBOL(ioctl_by_bdev);
/**
* lookup_bdev - lookup a struct block_device by name
* @pathname: special file representing the block device
+ * @mask: rights to check for (%MAY_READ, %MAY_WRITE, %MAY_EXEC)
*
* Get a reference to the blockdevice at @pathname in the current
* namespace if possible and return it. Return ERR_PTR(error)
- * otherwise.
+ * otherwise. If @mask is non-zero, check for access rights to the
+ * inode at @pathname.
*/
-struct block_device *lookup_bdev(const char *pathname)
+struct block_device *lookup_bdev(const char *pathname, int mask)
{
struct block_device *bdev;
struct inode *inode;
@@ -1726,6 +1733,11 @@ struct block_device *lookup_bdev(const char *pathname)
return ERR_PTR(error);
inode = d_backing_inode(path.dentry);
+ if (mask != 0 && !capable(CAP_SYS_ADMIN)) {
+ error = __inode_permission(inode, mask);
+ if (error)
+ goto fail;
+ }
error = -ENOTBLK;
if (!S_ISBLK(inode->i_mode))
goto fail;
diff --git a/fs/quota/quota.c b/fs/quota/quota.c
index 3746367098fd..a40eaecbd5cc 100644
--- a/fs/quota/quota.c
+++ b/fs/quota/quota.c
@@ -733,7 +733,7 @@ static struct super_block *quotactl_block(const char __user *special, int cmd)
if (IS_ERR(tmp))
return ERR_CAST(tmp);
- bdev = lookup_bdev(tmp->name);
+ bdev = lookup_bdev(tmp->name, 0);
putname(tmp);
if (IS_ERR(bdev))
return ERR_CAST(bdev);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 458ee7b213be..cc18dfb0b98e 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2388,7 +2388,7 @@ static inline void unregister_chrdev(unsigned int major, const char *name)
#define BLKDEV_MAJOR_HASH_SIZE 255
extern const char *__bdevname(dev_t, char *buffer);
extern const char *bdevname(struct block_device *bdev, char *buffer);
-extern struct block_device *lookup_bdev(const char *);
+extern struct block_device *lookup_bdev(const char *, int mask);
extern void blkdev_show(struct seq_file *,off_t);
#else
--
1.9.1
^ permalink raw reply related
* [PATCH 2/5] fs: Treat foreign mounts as nosuid
From: Seth Forshee @ 2015-09-30 20:15 UTC (permalink / raw)
To: Eric W. Biederman, Alexander Viro, Serge Hallyn, James Morris,
Serge E. Hallyn, Paul Moore, Stephen Smalley, Eric Paris
Cc: Andy Lutomirski, linux-fsdevel, linux-security-module, selinux,
linux-kernel, linux-mtd, linux-bcache, dm-devel, linux-raid,
Seth Forshee
In-Reply-To: <1443644116-41366-1-git-send-email-seth.forshee@canonical.com>
From: Andy Lutomirski <luto@amacapital.net>
If a process gets access to a mount from a different user
namespace, that process should not be able to take advantage of
setuid files or selinux entrypoints from that filesystem. Prevent
this by treating mounts from other mount namespaces and those not
owned by current_user_ns() or an ancestor as nosuid.
This will make it safer to allow more complex filesystems to be
mounted in non-root user namespaces.
This does not remove the need for MNT_LOCK_NOSUID. The setuid,
setgid, and file capability bits can no longer be abused if code in
a user namespace were to clear nosuid on an untrusted filesystem,
but this patch, by itself, is insufficient to protect the system
from abuse of files that, when execed, would increase MAC privilege.
As a more concrete explanation, any task that can manipulate a
vfsmount associated with a given user namespace already has
capabilities in that namespace and all of its descendents. If they
can cause a malicious setuid, setgid, or file-caps executable to
appear in that mount, then that executable will only allow them to
elevate privileges in exactly the set of namespaces in which they
are already privileges.
On the other hand, if they can cause a malicious executable to
appear with a dangerous MAC label, running it could change the
caller's security context in a way that should not have been
possible, even inside the namespace in which the task is confined.
As a hardening measure, this would have made CVE-2014-5207 much
more difficult to exploit.
Signed-off-by: Andy Lutomirski <luto@amacapital.net>
Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
---
fs/exec.c | 2 +-
fs/namespace.c | 13 +++++++++++++
include/linux/mount.h | 1 +
security/commoncap.c | 2 +-
security/selinux/hooks.c | 2 +-
5 files changed, 17 insertions(+), 3 deletions(-)
diff --git a/fs/exec.c b/fs/exec.c
index b06623a9347f..ea7311d72cc3 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1295,7 +1295,7 @@ static void bprm_fill_uid(struct linux_binprm *bprm)
bprm->cred->euid = current_euid();
bprm->cred->egid = current_egid();
- if (bprm->file->f_path.mnt->mnt_flags & MNT_NOSUID)
+ if (!mnt_may_suid(bprm->file->f_path.mnt))
return;
if (task_no_new_privs(current))
diff --git a/fs/namespace.c b/fs/namespace.c
index da70f7c4ece1..2101ce7b96ab 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -3276,6 +3276,19 @@ found:
return visible;
}
+bool mnt_may_suid(struct vfsmount *mnt)
+{
+ /*
+ * Foreign mounts (accessed via fchdir or through /proc
+ * symlinks) are always treated as if they are nosuid. This
+ * prevents namespaces from trusting potentially unsafe
+ * suid/sgid bits, file caps, or security labels that originate
+ * in other namespaces.
+ */
+ return !(mnt->mnt_flags & MNT_NOSUID) && check_mnt(real_mount(mnt)) &&
+ in_userns(current_user_ns(), mnt->mnt_sb->s_user_ns);
+}
+
static struct ns_common *mntns_get(struct task_struct *task)
{
struct ns_common *ns = NULL;
diff --git a/include/linux/mount.h b/include/linux/mount.h
index f822c3c11377..54a594d49733 100644
--- a/include/linux/mount.h
+++ b/include/linux/mount.h
@@ -81,6 +81,7 @@ extern void mntput(struct vfsmount *mnt);
extern struct vfsmount *mntget(struct vfsmount *mnt);
extern struct vfsmount *mnt_clone_internal(struct path *path);
extern int __mnt_is_readonly(struct vfsmount *mnt);
+extern bool mnt_may_suid(struct vfsmount *mnt);
struct path;
extern struct vfsmount *clone_private_mount(struct path *path);
diff --git a/security/commoncap.c b/security/commoncap.c
index 400aa224b491..6243aef5860e 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -448,7 +448,7 @@ static int get_file_caps(struct linux_binprm *bprm, bool *effective, bool *has_c
if (!file_caps_enabled)
return 0;
- if (bprm->file->f_path.mnt->mnt_flags & MNT_NOSUID)
+ if (!mnt_may_suid(bprm->file->f_path.mnt))
return 0;
if (!in_userns(current_user_ns(), bprm->file->f_path.mnt->mnt_sb->s_user_ns))
return 0;
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index e4369d86e588..de05207eb665 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -2171,7 +2171,7 @@ static int check_nnp_nosuid(const struct linux_binprm *bprm,
const struct task_security_struct *new_tsec)
{
int nnp = (bprm->unsafe & LSM_UNSAFE_NO_NEW_PRIVS);
- int nosuid = (bprm->file->f_path.mnt->mnt_flags & MNT_NOSUID);
+ int nosuid = !mnt_may_suid(bprm->file->f_path.mnt);
int rc;
if (!nnp && !nosuid)
--
1.9.1
^ permalink raw reply related
* [PATCH 3/5] selinux: Add support for unprivileged mounts from user namespaces
From: Seth Forshee @ 2015-09-30 20:15 UTC (permalink / raw)
To: Eric W. Biederman, Paul Moore, Stephen Smalley, Eric Paris
Cc: Alexander Viro, Serge Hallyn, Andy Lutomirski, linux-fsdevel,
linux-security-module, selinux, linux-kernel, linux-mtd,
linux-bcache, dm-devel, linux-raid, Seth Forshee, James Morris,
Serge E. Hallyn
In-Reply-To: <1443644116-41366-1-git-send-email-seth.forshee@canonical.com>
Security labels from unprivileged mounts in user namespaces must
be ignored. Force superblocks from user namespaces whose labeling
behavior is to use xattrs to use mountpoint labeling instead.
For the mountpoint label, default to converting the current task
context into a form suitable for file objects, but also allow the
policy writer to specify a different label through policy
transition rules.
Pieced together from code snippets provided by Stephen Smalley.
Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
---
security/selinux/hooks.c | 23 +++++++++++++++++++++++
1 file changed, 23 insertions(+)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index de05207eb665..09be1dc21e58 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -756,6 +756,28 @@ static int selinux_set_mnt_opts(struct super_block *sb,
goto out;
}
}
+
+ /*
+ * If this is a user namespace mount, no contexts are allowed
+ * on the command line and security labels must be ignored.
+ */
+ if (sb->s_user_ns != &init_user_ns) {
+ if (context_sid || fscontext_sid || rootcontext_sid ||
+ defcontext_sid) {
+ rc = -EACCES;
+ goto out;
+ }
+ if (sbsec->behavior == SECURITY_FS_USE_XATTR) {
+ sbsec->behavior = SECURITY_FS_USE_MNTPOINT;
+ rc = security_transition_sid(current_sid(), current_sid(),
+ SECCLASS_FILE, NULL,
+ &sbsec->mntpoint_sid);
+ if (rc)
+ goto out;
+ }
+ goto out_set_opts;
+ }
+
/* sets the context of the superblock for the fs being mounted. */
if (fscontext_sid) {
rc = may_context_mount_sb_relabel(fscontext_sid, sbsec, cred);
@@ -824,6 +846,7 @@ static int selinux_set_mnt_opts(struct super_block *sb,
sbsec->def_sid = defcontext_sid;
}
+out_set_opts:
rc = sb_finish_set_opts(sb);
out:
mutex_unlock(&sbsec->lock);
--
1.9.1
^ permalink raw reply related
* [PATCH 4/5] userns: Replace in_userns with current_in_userns
From: Seth Forshee @ 2015-09-30 20:15 UTC (permalink / raw)
To: Eric W. Biederman, Alexander Viro, Serge Hallyn, James Morris,
Serge E. Hallyn
Cc: Andy Lutomirski, linux-fsdevel, linux-security-module, selinux,
linux-kernel, linux-mtd, linux-bcache, dm-devel, linux-raid,
Seth Forshee
In-Reply-To: <1443644116-41366-1-git-send-email-seth.forshee@canonical.com>
All current callers of in_userns pass current_user_ns as the
first argument. Simplify by replacing in_userns with
current_in_userns which checks whether current_user_ns is in the
namespace supplied as an argument.
Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
---
fs/namespace.c | 2 +-
include/linux/user_namespace.h | 6 ++----
kernel/user_namespace.c | 6 +++---
security/commoncap.c | 2 +-
4 files changed, 7 insertions(+), 9 deletions(-)
diff --git a/fs/namespace.c b/fs/namespace.c
index 2101ce7b96ab..18fc58760aec 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -3286,7 +3286,7 @@ bool mnt_may_suid(struct vfsmount *mnt)
* in other namespaces.
*/
return !(mnt->mnt_flags & MNT_NOSUID) && check_mnt(real_mount(mnt)) &&
- in_userns(current_user_ns(), mnt->mnt_sb->s_user_ns);
+ current_in_userns(mnt->mnt_sb->s_user_ns);
}
static struct ns_common *mntns_get(struct task_struct *task)
diff --git a/include/linux/user_namespace.h b/include/linux/user_namespace.h
index a43faa727124..9217169c64cb 100644
--- a/include/linux/user_namespace.h
+++ b/include/linux/user_namespace.h
@@ -72,8 +72,7 @@ extern ssize_t proc_projid_map_write(struct file *, const char __user *, size_t,
extern ssize_t proc_setgroups_write(struct file *, const char __user *, size_t, loff_t *);
extern int proc_setgroups_show(struct seq_file *m, void *v);
extern bool userns_may_setgroups(const struct user_namespace *ns);
-extern bool in_userns(const struct user_namespace *ns,
- const struct user_namespace *target_ns);
+extern bool current_in_userns(const struct user_namespace *target_ns);
#else
static inline struct user_namespace *get_user_ns(struct user_namespace *ns)
@@ -103,8 +102,7 @@ static inline bool userns_may_setgroups(const struct user_namespace *ns)
return true;
}
-static inline bool in_userns(const struct user_namespace *ns,
- const struct user_namespace *target_ns)
+static inline bool current_in_userns(const struct user_namespace *target_ns)
{
return true;
}
diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c
index 69fbc377357b..5960edc7e644 100644
--- a/kernel/user_namespace.c
+++ b/kernel/user_namespace.c
@@ -949,10 +949,10 @@ bool userns_may_setgroups(const struct user_namespace *ns)
* Returns true if @ns is the same namespace as or a descendant of
* @target_ns.
*/
-bool in_userns(const struct user_namespace *ns,
- const struct user_namespace *target_ns)
+bool current_in_userns(const struct user_namespace *target_ns)
{
- for (; ns; ns = ns->parent) {
+ struct user_namespace *ns;
+ for (ns = current_user_ns(); ns; ns = ns->parent) {
if (ns == target_ns)
return true;
}
diff --git a/security/commoncap.c b/security/commoncap.c
index 6243aef5860e..2119421613f6 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -450,7 +450,7 @@ static int get_file_caps(struct linux_binprm *bprm, bool *effective, bool *has_c
if (!mnt_may_suid(bprm->file->f_path.mnt))
return 0;
- if (!in_userns(current_user_ns(), bprm->file->f_path.mnt->mnt_sb->s_user_ns))
+ if (!current_in_userns(bprm->file->f_path.mnt->mnt_sb->s_user_ns))
return 0;
rc = get_vfs_caps_from_disk(bprm->file->f_path.dentry, &vcaps);
--
1.9.1
^ permalink raw reply related
* [PATCH 5/5] Smack: Handle labels consistently in untrusted mounts
From: Seth Forshee @ 2015-09-30 20:15 UTC (permalink / raw)
To: Eric W. Biederman, Casey Schaufler
Cc: Alexander Viro, Serge Hallyn, Andy Lutomirski, linux-fsdevel,
linux-security-module, selinux, linux-kernel, linux-mtd,
linux-bcache, dm-devel, linux-raid, Seth Forshee, James Morris,
Serge E. Hallyn
In-Reply-To: <1443644116-41366-1-git-send-email-seth.forshee@canonical.com>
The SMACK64, SMACK64EXEC, and SMACK64MMAP labels are all handled
differently in untrusted mounts. This is confusing and
potentically problematic. Change this to handle them all the same
way that SMACK64 is currently handled; that is, read the label
from disk and check it at use time. For SMACK64 and SMACK64MMAP
access is denied if the label does not match smk_root. To be
consistent with suid, a SMACK64EXEC label which does not match
smk_root will still allow execution of the file but will not run
with the label supplied in the xattr.
Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
---
security/smack/smack_lsm.c | 28 ++++++++++++++++++----------
1 file changed, 18 insertions(+), 10 deletions(-)
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 621200f86b56..bee0b2652bf4 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -891,6 +891,7 @@ static int smack_bprm_set_creds(struct linux_binprm *bprm)
struct inode *inode = file_inode(bprm->file);
struct task_smack *bsp = bprm->cred->security;
struct inode_smack *isp;
+ struct superblock_smack *sbsp;
int rc;
if (bprm->cred_prepared)
@@ -900,6 +901,10 @@ static int smack_bprm_set_creds(struct linux_binprm *bprm)
if (isp->smk_task == NULL || isp->smk_task == bsp->smk_task)
return 0;
+ sbsp = inode->i_sb->s_security;
+ if (sbsp->smk_flags & SMK_SB_UNTRUSTED && isp->smk_task != sbsp->smk_root)
+ return 0;
+
if (bprm->unsafe & (LSM_UNSAFE_PTRACE | LSM_UNSAFE_PTRACE_CAP)) {
struct task_struct *tracer;
rc = 0;
@@ -1703,6 +1708,7 @@ static int smack_mmap_file(struct file *file,
struct task_smack *tsp;
struct smack_known *okp;
struct inode_smack *isp;
+ struct superblock_smack *sbsp;
int may;
int mmay;
int tmay;
@@ -1714,6 +1720,10 @@ static int smack_mmap_file(struct file *file,
isp = file_inode(file)->i_security;
if (isp->smk_mmap == NULL)
return 0;
+ sbsp = file_inode(file)->i_sb->s_security;
+ if (sbsp->smk_flags & SMK_SB_UNTRUSTED &&
+ isp->smk_mmap != sbsp->smk_root)
+ return -EACCES;
mkp = isp->smk_mmap;
tsp = current_security();
@@ -3492,16 +3502,14 @@ static void smack_d_instantiate(struct dentry *opt_dentry, struct inode *inode)
if (rc >= 0)
transflag = SMK_INODE_TRANSMUTE;
}
- if (!(sbsp->smk_flags & SMK_SB_UNTRUSTED)) {
- /*
- * Don't let the exec or mmap label be "*" or "@".
- */
- skp = smk_fetch(XATTR_NAME_SMACKEXEC, inode, dp);
- if (IS_ERR(skp) || skp == &smack_known_star ||
- skp == &smack_known_web)
- skp = NULL;
- isp->smk_task = skp;
- }
+ /*
+ * Don't let the exec or mmap label be "*" or "@".
+ */
+ skp = smk_fetch(XATTR_NAME_SMACKEXEC, inode, dp);
+ if (IS_ERR(skp) || skp == &smack_known_star ||
+ skp == &smack_known_web)
+ skp = NULL;
+ isp->smk_task = skp;
skp = smk_fetch(XATTR_NAME_SMACKMMAP, inode, dp);
if (IS_ERR(skp) || skp == &smack_known_star ||
--
1.9.1
^ permalink raw reply related
* [PATCH 0/3] raid5-cache fixes
From: Shaohua Li @ 2015-09-30 23:15 UTC (permalink / raw)
To: linux-raid; +Cc: Kernel-team, songliubraving, hch, dan.j.williams, neilb
Neil,
These are some fixes for raid5-cache. The first one fixes the issue related to
trim. The last two make raid5-cache support log device IO error.
Thanks,
Shaohua
Shaohua Li (3):
raid5-cache: add trim support for log
md: don't export log device
raid5-cache: IO error handling
drivers/md/md.c | 4 ++--
drivers/md/raid5-cache.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++--
2 files changed, 62 insertions(+), 4 deletions(-)
--
2.4.6
^ permalink raw reply
* [PATCH 1/3] raid5-cache: add trim support for log
From: Shaohua Li @ 2015-09-30 23:15 UTC (permalink / raw)
To: linux-raid; +Cc: Kernel-team, songliubraving, hch, dan.j.williams, neilb
In-Reply-To: <cover.1443653794.git.shli@fb.com>
Since superblock is updated infrequently, we do a simple trim of log
disk (a synchronous trim)
Signed-off-by: Shaohua Li <shli@fb.com>
---
drivers/md/raid5-cache.c | 44 +++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 43 insertions(+), 1 deletion(-)
diff --git a/drivers/md/raid5-cache.c b/drivers/md/raid5-cache.c
index a02f9ce..afc3b6b 100644
--- a/drivers/md/raid5-cache.c
+++ b/drivers/md/raid5-cache.c
@@ -654,6 +654,48 @@ static void r5l_kick_io_unit(struct r5l_log *log)
}
static void r5l_write_super(struct r5l_log *log, sector_t cp);
+static void r5l_write_super_and_discard_space(struct r5l_log *log,
+ sector_t end)
+{
+ struct block_device *bdev = log->rdev->bdev;
+ struct mddev *mddev;
+
+ r5l_write_super(log, end);
+
+ if (!blk_queue_discard(bdev_get_queue(bdev)))
+ return;
+
+ /* discard destroy old data in log, so force a super update */
+ mddev = log->rdev->mddev;
+ /*
+ * mddev->thread could be shut down already in raid array stop. At that
+ * time, we should already lock reconfig_mutex
+ * */
+ if (!mddev->thread) {
+ WARN_ON(!mddev_is_locked(mddev));
+ md_update_sb(mddev, 1);
+ } else {
+ set_bit(MD_CHANGE_PENDING, &mddev->flags);
+ md_wakeup_thread(mddev->thread);
+ wait_event(mddev->sb_wait,
+ !test_bit(MD_CHANGE_PENDING, &mddev->flags));
+ }
+
+ if (log->last_checkpoint < end) {
+ blkdev_issue_discard(bdev,
+ log->last_checkpoint + log->rdev->data_offset,
+ end - log->last_checkpoint, GFP_NOIO, 0);
+ } else {
+ blkdev_issue_discard(bdev,
+ log->last_checkpoint + log->rdev->data_offset,
+ log->device_size - log->last_checkpoint,
+ GFP_NOIO, 0);
+ blkdev_issue_discard(bdev, log->rdev->data_offset, end,
+ GFP_NOIO, 0);
+ }
+}
+
+
static void r5l_do_reclaim(struct r5l_log *log)
{
struct r5l_io_unit *io, *last;
@@ -709,7 +751,7 @@ static void r5l_do_reclaim(struct r5l_log *log)
* here, because the log area might be reused soon and we don't want to
* confuse recovery
*/
- r5l_write_super(log, last->log_start);
+ r5l_write_super_and_discard_space(log, last->log_start);
mutex_lock(&log->io_mutex);
log->last_checkpoint = last->log_start;
--
2.4.6
^ permalink raw reply related
* [PATCH 2/3] md: don't export log device
From: Shaohua Li @ 2015-09-30 23:15 UTC (permalink / raw)
To: linux-raid; +Cc: Kernel-team, songliubraving, hch, dan.j.williams, neilb
In-Reply-To: <cover.1443653794.git.shli@fb.com>
If there is IO error in log device, don't export it like other raid
disks. Otherwise we get kernel crash in different places since
rdev->bdev, rdev->mddev becomes NULL
Signed-off-by: Shaohua Li <shli@fb.com>
---
drivers/md/md.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/md/md.c b/drivers/md/md.c
index f1cbb08..0b1d7ef 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -2519,7 +2519,7 @@ state_store(struct md_rdev *rdev, const char *buf, size_t len)
else
err = -EBUSY;
} else if (cmd_match(buf, "remove")) {
- if (rdev->raid_disk >= 0)
+ if (rdev->raid_disk >= 0 || test_bit(Journal, &rdev->flags))
err = -EBUSY;
else {
struct mddev *mddev = rdev->mddev;
@@ -6040,7 +6040,7 @@ static int hot_remove_disk(struct mddev *mddev, dev_t dev)
clear_bit(Blocked, &rdev->flags);
remove_and_add_spares(mddev, rdev);
- if (rdev->raid_disk >= 0)
+ if (rdev->raid_disk >= 0 || test_bit(Journal, &rdev->flags))
goto busy;
if (mddev_is_clustered(mddev))
--
2.4.6
^ permalink raw reply related
* [PATCH 3/3] raid5-cache: IO error handling
From: Shaohua Li @ 2015-09-30 23:15 UTC (permalink / raw)
To: linux-raid; +Cc: Kernel-team, songliubraving, hch, dan.j.williams, neilb
In-Reply-To: <cover.1443653794.git.shli@fb.com>
There are 3 places the raid5-cache dispatches IO. The discard IO error
doesn't matter, so we ignore it. The superblock write IO error can be
handled in MD core. The remaining are log write and flush. When the IO
error happens, we simply fail all raid disks and continue the stripe
state machine. The MD/raid5 core can handle it (for example, mark all
disks faulty, report bio error and so on).
Signed-off-by: Shaohua Li <shli@fb.com>
---
drivers/md/raid5-cache.c | 18 +++++++++++++++++-
1 file changed, 17 insertions(+), 1 deletion(-)
diff --git a/drivers/md/raid5-cache.c b/drivers/md/raid5-cache.c
index afc3b6b..430ce5c 100644
--- a/drivers/md/raid5-cache.c
+++ b/drivers/md/raid5-cache.c
@@ -223,7 +223,16 @@ static void __r5l_set_io_unit_state(struct r5l_io_unit *io,
io->state = state;
}
-/* XXX: totally ignores I/O errors */
+static void r5l_log_io_error(struct r5l_log *log)
+{
+ struct md_rdev *rdev;
+
+ rcu_read_lock();
+ rdev_for_each_rcu(rdev, log->rdev->mddev)
+ md_error(log->rdev->mddev, rdev);
+ rcu_read_unlock();
+}
+
static void r5l_log_endio(struct bio *bio)
{
struct r5l_io_unit *io = bio->bi_private;
@@ -232,6 +241,9 @@ static void r5l_log_endio(struct bio *bio)
bio_put(bio);
+ if (bio->bi_error)
+ r5l_log_io_error(log);
+
if (!atomic_dec_and_test(&io->pending_io))
return;
@@ -594,6 +606,9 @@ static void r5l_log_flush_endio(struct bio *bio)
struct r5l_io_unit *io;
struct stripe_head *sh;
+ if (bio->bi_error)
+ r5l_log_io_error(log);
+
spin_lock_irqsave(&log->io_list_lock, flags);
list_for_each_entry(io, &log->flushing_ios, log_sibling) {
while (!list_empty(&io->stripe_list)) {
@@ -681,6 +696,7 @@ static void r5l_write_super_and_discard_space(struct r5l_log *log,
!test_bit(MD_CHANGE_PENDING, &mddev->flags));
}
+ /* discard IO error really doesn't matter, ignore it */
if (log->last_checkpoint < end) {
blkdev_issue_discard(bdev,
log->last_checkpoint + log->rdev->data_offset,
--
2.4.6
^ permalink raw reply related
* Re: [PATCH 1/5] fs: Verify access of user towards block device file when mounting
From: Mike Snitzer @ 2015-09-30 23:42 UTC (permalink / raw)
To: Seth Forshee
Cc: Eric W. Biederman, Kent Overstreet, Alasdair Kergon, dm-devel,
Neil Brown, David Woodhouse, Brian Norris, Alexander Viro,
Jan Kara, Jeff Layton, J. Bruce Fields, Serge Hallyn,
Andy Lutomirski, linux-fsdevel, linux-security-module, selinux,
linux-kernel, linux-mtd, linux-bcache, linux-raid
In-Reply-To: <1443644116-41366-2-git-send-email-seth.forshee@canonical.com>
On Wed, Sep 30 2015 at 4:15pm -0400,
Seth Forshee <seth.forshee@canonical.com> wrote:
> When mounting a filesystem on a block device there is currently
> no verification that the user has appropriate access to the
> device file passed to mount. This has not been an issue so far
> since the user in question has always been root, but this must
> be changed before allowing unprivileged users to mount in user
> namespaces.
>
> To fix this, add an argument to lookup_bdev() to specify the
> required permissions. If the mask of permissions is zero, or
> if the user has CAP_SYS_ADMIN, the permission check is skipped,
> otherwise the lookup fails if the user does not have the
> specified access rights for the inode at the supplied path.
>
> Callers associated with mounting are updated to pass permission
> masks to lookup_bdev() so that these mounts will fail for an
> unprivileged user who lacks permissions for the block device
> inode. All other callers pass 0 to maintain their current
> behaviors.
>
> Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
> ---
> drivers/md/bcache/super.c | 2 +-
> drivers/md/dm-table.c | 2 +-
> drivers/mtd/mtdsuper.c | 6 +++++-
> fs/block_dev.c | 18 +++++++++++++++---
> fs/quota/quota.c | 2 +-
> include/linux/fs.h | 2 +-
> 6 files changed, 24 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
> index e76ed003769e..35bb3ea4cbe2 100644
> --- a/drivers/md/dm-table.c
> +++ b/drivers/md/dm-table.c
> @@ -380,7 +380,7 @@ int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode,
> BUG_ON(!t);
>
> /* convert the path to a device */
> - bdev = lookup_bdev(path);
> + bdev = lookup_bdev(path, 0);
> if (IS_ERR(bdev)) {
> dev = name_to_dev_t(path);
> if (!dev)
Given dm_get_device() is passed @mode why not have it do something like
you did in blkdev_get_by_path()? e.g.:
> diff --git a/fs/block_dev.c b/fs/block_dev.c
> index 26cee058dc02..54d94cd64577 100644
> --- a/fs/block_dev.c
> +++ b/fs/block_dev.c
> @@ -1394,9 +1394,14 @@ struct block_device *blkdev_get_by_path(const char *path, fmode_t mode,
> void *holder)
> {
> struct block_device *bdev;
> + int perm = 0;
> int err;
>
> - bdev = lookup_bdev(path);
> + if (mode & FMODE_READ)
> + perm |= MAY_READ;
> + if (mode & FMODE_WRITE)
> + perm |= MAY_WRITE;
> + bdev = lookup_bdev(path, perm);
> if (IS_ERR(bdev))
> return bdev;
>
^ permalink raw reply
* Re: [PATCH 1/3] raid5-cache: add trim support for log
From: Neil Brown @ 2015-10-01 4:41 UTC (permalink / raw)
To: Shaohua Li, linux-raid; +Cc: Kernel-team, songliubraving, hch, dan.j.williams
In-Reply-To: <9ee5464d20a78732ba4dda9c0601ea10d47f0e03.1443653794.git.shli@fb.com>
[-- Attachment #1: Type: text/plain, Size: 2643 bytes --]
Shaohua Li <shli@fb.com> writes:
> Since superblock is updated infrequently, we do a simple trim of log
> disk (a synchronous trim)
>
> Signed-off-by: Shaohua Li <shli@fb.com>
> ---
> drivers/md/raid5-cache.c | 44 +++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 43 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/md/raid5-cache.c b/drivers/md/raid5-cache.c
> index a02f9ce..afc3b6b 100644
> --- a/drivers/md/raid5-cache.c
> +++ b/drivers/md/raid5-cache.c
> @@ -654,6 +654,48 @@ static void r5l_kick_io_unit(struct r5l_log *log)
> }
>
> static void r5l_write_super(struct r5l_log *log, sector_t cp);
> +static void r5l_write_super_and_discard_space(struct r5l_log *log,
> + sector_t end)
> +{
> + struct block_device *bdev = log->rdev->bdev;
> + struct mddev *mddev;
> +
> + r5l_write_super(log, end);
> +
> + if (!blk_queue_discard(bdev_get_queue(bdev)))
> + return;
> +
> + /* discard destroy old data in log, so force a super update */
> + mddev = log->rdev->mddev;
> + /*
> + * mddev->thread could be shut down already in raid array stop. At that
> + * time, we should already lock reconfig_mutex
> + * */
> + if (!mddev->thread) {
> + WARN_ON(!mddev_is_locked(mddev));
> + md_update_sb(mddev, 1);
> + } else {
> + set_bit(MD_CHANGE_PENDING, &mddev->flags);
> + md_wakeup_thread(mddev->thread);
> + wait_event(mddev->sb_wait,
> + !test_bit(MD_CHANGE_PENDING, &mddev->flags));
> + }
I didn't like this. It looks clumsy to me.
So I went to have a look at the code to understand why it was needed.
I found that r5l_exit_log() was being called from free_conf().
I didn't notice that before.
->free() is only supposed to free, not write anything. It could be too
late to write anything.
You need to get the raid5_quiesce(1) call to stablise the array. It can
do the final r5l_do_reclaim().
So it is OK for free_conf to call r5l_exit_log as long as it only
deregisters the thread and frees the data structures.
The "r5l_do_reclaim" needs to be moved out.
I wonder where the md_update_sb() should go...
We currently calls "stop_writes" and then "mddev_detach".
So we shouldn't be writing anything by the time we get to mddev_detach,
but in there we wait for writes to complete. That looks wrong.
I might move some of that stuff from mddev_detach to __md_stop_writes.
Then md_stop_writes can call ->quiesce and then md_update_sb(). That is
enough for raid5-cache to just call r5l_do_reclaim in raid5_quiesce, and
have the md_update_sb() called at the right time.
I'll see if i can make that work.
Thanks,
NeilBrown
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]
^ permalink raw reply
* Re: [PATCH 2/3] md: don't export log device
From: Neil Brown @ 2015-10-01 4:45 UTC (permalink / raw)
To: Shaohua Li, linux-raid; +Cc: Kernel-team, songliubraving, hch, dan.j.williams
In-Reply-To: <78d0cd55e9c1476869933454e06b3f74ad88ce9c.1443653794.git.shli@fb.com>
[-- Attachment #1: Type: text/plain, Size: 1351 bytes --]
Shaohua Li <shli@fb.com> writes:
> If there is IO error in log device, don't export it like other raid
> disks. Otherwise we get kernel crash in different places since
> rdev->bdev, rdev->mddev becomes NULL
>
> Signed-off-by: Shaohua Li <shli@fb.com>
> ---
> drivers/md/md.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index f1cbb08..0b1d7ef 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -2519,7 +2519,7 @@ state_store(struct md_rdev *rdev, const char *buf, size_t len)
> else
> err = -EBUSY;
> } else if (cmd_match(buf, "remove")) {
> - if (rdev->raid_disk >= 0)
> + if (rdev->raid_disk >= 0 || test_bit(Journal, &rdev->flags))
> err = -EBUSY;
> else {
> struct mddev *mddev = rdev->mddev;
> @@ -6040,7 +6040,7 @@ static int hot_remove_disk(struct mddev *mddev, dev_t dev)
> clear_bit(Blocked, &rdev->flags);
> remove_and_add_spares(mddev, rdev);
>
> - if (rdev->raid_disk >= 0)
> + if (rdev->raid_disk >= 0 || test_bit(Journal, &rdev->flags))
> goto busy;
>
> if (mddev_is_clustered(mddev))
> --
> 2.4.6
I'd rather keep these details local to the RAID5 code...
Can we just have raid5_remove_disk return -EBUSY when an attempt is made
to remove the Journal device?
Thanks,
NeilBrown
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]
^ permalink raw reply
* Re: [PATCH 3/3] raid5-cache: IO error handling
From: Neil Brown @ 2015-10-01 4:50 UTC (permalink / raw)
To: Shaohua Li, linux-raid; +Cc: Kernel-team, songliubraving, hch, dan.j.williams
In-Reply-To: <e9c915cf18d81f76e122a2533baeb68c56186d8f.1443653794.git.shli@fb.com>
[-- Attachment #1: Type: text/plain, Size: 2529 bytes --]
Shaohua Li <shli@fb.com> writes:
> There are 3 places the raid5-cache dispatches IO. The discard IO error
> doesn't matter, so we ignore it. The superblock write IO error can be
> handled in MD core. The remaining are log write and flush. When the IO
> error happens, we simply fail all raid disks and continue the stripe
> state machine. The MD/raid5 core can handle it (for example, mark all
> disks faulty, report bio error and so on).
>
> Signed-off-by: Shaohua Li <shli@fb.com>
> ---
> drivers/md/raid5-cache.c | 18 +++++++++++++++++-
> 1 file changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/md/raid5-cache.c b/drivers/md/raid5-cache.c
> index afc3b6b..430ce5c 100644
> --- a/drivers/md/raid5-cache.c
> +++ b/drivers/md/raid5-cache.c
> @@ -223,7 +223,16 @@ static void __r5l_set_io_unit_state(struct r5l_io_unit *io,
> io->state = state;
> }
>
> -/* XXX: totally ignores I/O errors */
> +static void r5l_log_io_error(struct r5l_log *log)
> +{
> + struct md_rdev *rdev;
> +
> + rcu_read_lock();
> + rdev_for_each_rcu(rdev, log->rdev->mddev)
> + md_error(log->rdev->mddev, rdev);
> + rcu_read_unlock();
> +}
This fails spare devices too... seems a bit heavy handed.
If the journal device fails we should still be able to read from the
array, just not write.
So can we just enhance the
if (s.failed > conf->max_degraded) {
test in handle_stripe(), and probably improve has_failed() too??
Thanks,
NeilBrown
> +
> static void r5l_log_endio(struct bio *bio)
> {
> struct r5l_io_unit *io = bio->bi_private;
> @@ -232,6 +241,9 @@ static void r5l_log_endio(struct bio *bio)
>
> bio_put(bio);
>
> + if (bio->bi_error)
> + r5l_log_io_error(log);
> +
> if (!atomic_dec_and_test(&io->pending_io))
> return;
>
> @@ -594,6 +606,9 @@ static void r5l_log_flush_endio(struct bio *bio)
> struct r5l_io_unit *io;
> struct stripe_head *sh;
>
> + if (bio->bi_error)
> + r5l_log_io_error(log);
> +
> spin_lock_irqsave(&log->io_list_lock, flags);
> list_for_each_entry(io, &log->flushing_ios, log_sibling) {
> while (!list_empty(&io->stripe_list)) {
> @@ -681,6 +696,7 @@ static void r5l_write_super_and_discard_space(struct r5l_log *log,
> !test_bit(MD_CHANGE_PENDING, &mddev->flags));
> }
>
> + /* discard IO error really doesn't matter, ignore it */
> if (log->last_checkpoint < end) {
> blkdev_issue_discard(bdev,
> log->last_checkpoint + log->rdev->data_offset,
> --
> 2.4.6
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]
^ permalink raw reply
* Re: [PATCH 1/5] fs: Verify access of user towards block device file when mounting
From: Seth Forshee @ 2015-10-01 12:55 UTC (permalink / raw)
To: Mike Snitzer
Cc: Eric W. Biederman, Kent Overstreet, Alasdair Kergon, dm-devel,
Neil Brown, David Woodhouse, Brian Norris, Alexander Viro,
Jan Kara, Jeff Layton, J. Bruce Fields, Serge Hallyn,
Andy Lutomirski, linux-fsdevel, linux-security-module, selinux,
linux-kernel, linux-mtd, linux-bcache, linux-raid
In-Reply-To: <20150930234215.GA24127@redhat.com>
On Wed, Sep 30, 2015 at 07:42:15PM -0400, Mike Snitzer wrote:
> On Wed, Sep 30 2015 at 4:15pm -0400,
> Seth Forshee <seth.forshee@canonical.com> wrote:
>
> > When mounting a filesystem on a block device there is currently
> > no verification that the user has appropriate access to the
> > device file passed to mount. This has not been an issue so far
> > since the user in question has always been root, but this must
> > be changed before allowing unprivileged users to mount in user
> > namespaces.
> >
> > To fix this, add an argument to lookup_bdev() to specify the
> > required permissions. If the mask of permissions is zero, or
> > if the user has CAP_SYS_ADMIN, the permission check is skipped,
> > otherwise the lookup fails if the user does not have the
> > specified access rights for the inode at the supplied path.
> >
> > Callers associated with mounting are updated to pass permission
> > masks to lookup_bdev() so that these mounts will fail for an
> > unprivileged user who lacks permissions for the block device
> > inode. All other callers pass 0 to maintain their current
> > behaviors.
> >
> > Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
> > ---
> > drivers/md/bcache/super.c | 2 +-
> > drivers/md/dm-table.c | 2 +-
> > drivers/mtd/mtdsuper.c | 6 +++++-
> > fs/block_dev.c | 18 +++++++++++++++---
> > fs/quota/quota.c | 2 +-
> > include/linux/fs.h | 2 +-
> > 6 files changed, 24 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
> > index e76ed003769e..35bb3ea4cbe2 100644
> > --- a/drivers/md/dm-table.c
> > +++ b/drivers/md/dm-table.c
> > @@ -380,7 +380,7 @@ int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode,
> > BUG_ON(!t);
> >
> > /* convert the path to a device */
> > - bdev = lookup_bdev(path);
> > + bdev = lookup_bdev(path, 0);
> > if (IS_ERR(bdev)) {
> > dev = name_to_dev_t(path);
> > if (!dev)
>
> Given dm_get_device() is passed @mode why not have it do something like
> you did in blkdev_get_by_path()? e.g.:
I only dealt with code related to mounting in this patch since that's
what I'm working on. I have it on my TODO list to consider converting
other callers of lookup_bdev. But if you're sure doing so makes sense
for dm_get_device and that it won't cause regressions then I could add a
patch for it.
Thanks,
Seth
^ permalink raw reply
* Re: [PATCH 1/5] fs: Verify access of user towards block device file when mounting
From: Mike Snitzer @ 2015-10-01 13:40 UTC (permalink / raw)
To: Seth Forshee
Cc: Eric W. Biederman, Kent Overstreet, Alasdair Kergon, dm-devel,
Neil Brown, David Woodhouse, Brian Norris, Alexander Viro,
Jan Kara, Jeff Layton, J. Bruce Fields, Serge Hallyn,
Andy Lutomirski, linux-fsdevel, linux-security-module, selinux,
linux-kernel, linux-mtd, linux-bcache, linux-raid
In-Reply-To: <20151001125508.GA101875@ubuntu-hedt>
On Thu, Oct 01 2015 at 8:55am -0400,
Seth Forshee <seth.forshee@canonical.com> wrote:
> On Wed, Sep 30, 2015 at 07:42:15PM -0400, Mike Snitzer wrote:
> > On Wed, Sep 30 2015 at 4:15pm -0400,
> > Seth Forshee <seth.forshee@canonical.com> wrote:
> >
> > > When mounting a filesystem on a block device there is currently
> > > no verification that the user has appropriate access to the
> > > device file passed to mount. This has not been an issue so far
> > > since the user in question has always been root, but this must
> > > be changed before allowing unprivileged users to mount in user
> > > namespaces.
> > >
> > > To fix this, add an argument to lookup_bdev() to specify the
> > > required permissions. If the mask of permissions is zero, or
> > > if the user has CAP_SYS_ADMIN, the permission check is skipped,
> > > otherwise the lookup fails if the user does not have the
> > > specified access rights for the inode at the supplied path.
> > >
> > > Callers associated with mounting are updated to pass permission
> > > masks to lookup_bdev() so that these mounts will fail for an
> > > unprivileged user who lacks permissions for the block device
> > > inode. All other callers pass 0 to maintain their current
> > > behaviors.
> > >
> > > Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
> > > ---
> > > drivers/md/bcache/super.c | 2 +-
> > > drivers/md/dm-table.c | 2 +-
> > > drivers/mtd/mtdsuper.c | 6 +++++-
> > > fs/block_dev.c | 18 +++++++++++++++---
> > > fs/quota/quota.c | 2 +-
> > > include/linux/fs.h | 2 +-
> > > 6 files changed, 24 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
> > > index e76ed003769e..35bb3ea4cbe2 100644
> > > --- a/drivers/md/dm-table.c
> > > +++ b/drivers/md/dm-table.c
> > > @@ -380,7 +380,7 @@ int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode,
> > > BUG_ON(!t);
> > >
> > > /* convert the path to a device */
> > > - bdev = lookup_bdev(path);
> > > + bdev = lookup_bdev(path, 0);
> > > if (IS_ERR(bdev)) {
> > > dev = name_to_dev_t(path);
> > > if (!dev)
> >
> > Given dm_get_device() is passed @mode why not have it do something like
> > you did in blkdev_get_by_path()? e.g.:
>
> I only dealt with code related to mounting in this patch since that's
> what I'm working on. I have it on my TODO list to consider converting
> other callers of lookup_bdev. But if you're sure doing so makes sense
> for dm_get_device and that it won't cause regressions then I could add a
> patch for it.
OK, dm_get_device() is called in DM device activation path (by tools
like lvm2).
After lookup_bdev() it goes on to call blkdev_get_by_dev() with this
call chain:
dm_get_device -> dm_get_table_device -> open_table_device -> blkdev_get_by_dev
Not immediately clear to me why we'd need to augment blkdev_get_by_dev()
to do this checking also.
However, thinking further: In a device stack (e.g. dm/lvm2, md, etc)
new virtual block devices are created that layer ontop of the
traditional block devices. This level of indirection may cause your
lookup_bdev() check to go on to succeed (if access constraints were not
established on the upper level dm or md device?). I'm just thinking
outloud here: but have you verified your changes work as intended on
devices created with either lvm2 or mdadm?
What layer establishes access rights to historically root-only
priviledged block devices? Is it user namespaces?
I haven't kept up with user namespaces as it relates to stacking block
drivers like DM. But I'm happy to come up to speed and at the same time
help you verify all works as expected with DM blocks devices...
Mike
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox