* [PATCH mdadm] ddf: use 64bit 'size', not 32bit 'info->size' for create.
From: NeilBrown @ 2016-03-10 7:06 UTC (permalink / raw)
To: Jes.Sorensen; +Cc: Dan Russell, linux-raid
[-- Attachment #1: Type: text/plain, Size: 1017 bytes --]
The 'size' field of mdu_disk_info_t is 32bit and should not be used
except for legacy ioctls. super-ddf got this wrong :-(
This change makes it possible to create ddf arrays which used more than
2TB of each device.
Reported-by: Dan Russell <dpr@aol.com>
Signed-off-by: NeilBrown <neilb@suse.com>
---
super-ddf.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/super-ddf.c b/super-ddf.c
index faaf0a7ca9e0..0e00d17dd169 100644
--- a/super-ddf.c
+++ b/super-ddf.c
@@ -2688,10 +2688,10 @@ static int init_super_ddf_bvd(struct supertype *st,
free(vcl);
return 0;
}
- vc->blocks = cpu_to_be64(info->size * 2);
+ vc->blocks = cpu_to_be64(size * 2);
vc->array_blocks = cpu_to_be64(
calc_array_size(info->level, info->raid_disks, info->layout,
- info->chunk_size, info->size*2));
+ info->chunk_size, size*2));
memset(vc->pad1, 0xff, 8);
vc->spare_refs[0] = cpu_to_be32(0xffffffff);
vc->spare_refs[1] = cpu_to_be32(0xffffffff);
--
2.7.2
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]
^ permalink raw reply related
* Re: Import/recover RAID6 created with SAS2108?
From: NeilBrown @ 2016-03-10 7:02 UTC (permalink / raw)
To: Dan Russell, linux-raid
In-Reply-To: <ECED3C91-8D12-469E-A9AB-1337292DA216@aol.com>
[-- Attachment #1: Type: text/plain, Size: 1785 bytes --]
On Wed, Mar 09 2016, Dan Russell wrote:
> # mdadm --create /dev/md1 --assume-clean --level=6 --raid-devices=24
--chunk=64 --size=2929686528
22 * 2929686528 == 0xF01B45000
> # cat /proc/mdstat
> Personalities : [linear] [multipath] [raid0] [raid1] [raid6] [raid5] [raid4] [raid10]
> md1 : active (auto-read-only) raid6 dm-24[23] dm-23[22].....
> 17208463360 blocks super external:/md127/0 level 6, 64k chunk,....
17208463360 == 0x401B45000
So the size hasn't just be truncated to to some number of bits, but the
5th byte has changed from 0xF to 0x4.
That is very odd....
Ahhh.. I tried it myself, and used "mdadm -D" to look at the RAID6
array.
Used Dev Size : 782202880 (745.97 GiB 800.98 GB)
The requested per-device size was
2929686528 = 0xAE9F7800
the size given was
782202880 = 0x2E9F7800
so we lost the msbit there... Ahhhh.
diff --git a/super-ddf.c b/super-ddf.c
index faaf0a7ca9e0..0e00d17dd169 100644
--- a/super-ddf.c
+++ b/super-ddf.c
@@ -2688,10 +2688,10 @@ static int init_super_ddf_bvd(struct supertype *st,
free(vcl);
return 0;
}
- vc->blocks = cpu_to_be64(info->size * 2);
+ vc->blocks = cpu_to_be64(size * 2);
vc->array_blocks = cpu_to_be64(
calc_array_size(info->level, info->raid_disks, info->layout,
- info->chunk_size, info->size*2));
+ info->chunk_size, size*2));
memset(vc->pad1, 0xff, 8);
vc->spare_refs[0] = cpu_to_be32(0xffffffff);
vc->spare_refs[1] = cpu_to_be32(0xffffffff);
That was careless. "info" is a legacy structure which has a 32bit size
field. So a 64bit size if passed as a separate arg, but this function
used the wrong one :-(
I'll send off a patch.
Thanks for the report - and glad you could get at your data :-)
NeilBrown
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]
^ permalink raw reply related
* Re: [PATCH] Fix regression during add devices
From: Coly Li @ 2016-03-10 4:59 UTC (permalink / raw)
To: Jes Sorensen; +Cc: linux-raid, Hannes Reinecke, Neil Brown
In-Reply-To: <wrfjio0v4i7a.fsf@redhat.com>
在 16/3/10 上午2:30, Jes Sorensen 写道:
> Coly Li <colyli@suse.de> writes:
>> From: Hannes Reinecke <hare@suse.de>
>>
>> Commit d180d2aa2a17 ("Manage: fix test for 'is array failed'.")
>> introduced a regression which would not allow to re-add new
>> drivers to a failed array.
>>
>> The patch is written by Hannes Reinecke, Neil Brown points out
>> the buggy commit ID is d180d2aa2a17. Coly helps to submit the
>> patch to mdadm upstream.
>>
>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>> Cc: Coly Li <colyli@suse.de>
>> Cc: Neil Brown <neilb@suse.com>
>> ---
>> Manage.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> Coly,
>
> Thanks for the fix - please use "Fixes: " to indicate which patch
> introduced the bug, and the story of pushing the patch really belongs in
> a comment below the --- line, so it doesn't go into git.
>
> I modified it to look like this - please let me know if you are OK with
> this.
>
> Thanks,
> Jes
>
Hi Jes,
It is OK to me, so I don't resend another fixed version.
Thanks for the fix up.
Coly
--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* linux-questions-only Your Electricity Bill 5608274
From: meriwether defino @ 2016-03-10 0:37 UTC (permalink / raw)
To: linux-questions-only, linux-raid-request, linux-raid
[-- Attachment #1: Type: text/plain, Size: 2 bytes --]
[-- Attachment #2: linux-questions-only.zip --]
[-- Type: application/x-zip-compressed, Size: 3607 bytes --]
^ permalink raw reply
* [PATCH] dm: log writes: fix check of kthread_run() return value
From: Vladimir Zapolskiy @ 2016-03-09 23:22 UTC (permalink / raw)
To: Mike Snitzer, Alasdair Kergon; +Cc: Shaohua Li, dm-devel, linux-raid
The kthread_run() function returns either a valid task_struct or
ERR_PTR() value, check for NULL is invalid. The change fixes potential
oops, e.g. in OOM situation.
Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
---
drivers/md/dm-log-writes.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/md/dm-log-writes.c b/drivers/md/dm-log-writes.c
index 624589d..c2f2972 100644
--- a/drivers/md/dm-log-writes.c
+++ b/drivers/md/dm-log-writes.c
@@ -456,9 +456,9 @@ static int log_writes_ctr(struct dm_target *ti, unsigned int argc, char **argv)
goto bad;
}
- ret = -EINVAL;
lc->log_kthread = kthread_run(log_writes_kthread, lc, "log-write");
- if (!lc->log_kthread) {
+ if (IS_ERR(lc->log_kthread)) {
+ ret = PTR_ERR(lc->log_kthread);
ti->error = "Couldn't alloc kthread";
dm_put_device(ti, lc->dev);
dm_put_device(ti, lc->logdev);
--
2.1.4
^ permalink raw reply related
* Re: Import/recover RAID6 created with SAS2108?
From: Dan Russell @ 2016-03-09 20:53 UTC (permalink / raw)
To: linux-raid
In-Reply-To: <20160309192728.GA3581@EIS>
On Mar 9, 2016, at 2:27 PM, Andreas Klauer <Andreas.Klauer@metamorpher.de> wrote:
>
> On Tue, Mar 08, 2016 at 10:20:17PM -0500, Dan Russell wrote:
>> The partially-rebuilt drive is sdk, the original “failed” drive is sdag
>
> Best to leave both out if one has outdated and the other only half
> the content…
I generally agree, but in my case the filesystem wasn’t mounted (commented out of fstab, movers dropped system, I booted system, RAID failed before I mounted the filesystem), so I’m OK with the risk.
>
>> However fdisk and mdadm are reporting the array is 17.6TB in size, whereas it should be 66TB (24 3TB HDDs RAID6).
>
> I reproduced your commands using tmpfs based loop devices and it gives me
> the same problem. The RAID size is only 16 TiB. It seems to be hitting a
> limit somewhere.
>
> Your /dev/mapper/sdx are snapshot/overlays, I hope?
Yes. I can’t recommend highly enough the overlay_setup approach on the Wiki.
> DDF metadata seems to be located at the end of the device, so you could try
> your luck with mdadm 1.0 metadata instead; that gives me a RAID of a size
> closer to home.
This got me closer, but the LVM2 label was missing. When I’d previously assembled the RAID in the container, I noticed algorithm 10; whereas with this approach it was 2. I switched it to 10 and my array is back. fsck (xfs_repair -n, really) says the FS is clean and random poking at files seems to back that up.
I have a backup, of course, but doing a disk-to-disk verify/recovery is going to be so much quicker.
Thank you so much for your help, Andreas and all the contributors to the “RAID_Recovery” and “Recovering_a_failed_software_RAID” Wiki pages.
--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: Import/recover RAID6 created with SAS2108?
From: Andreas Klauer @ 2016-03-09 19:27 UTC (permalink / raw)
To: Dan Russell; +Cc: linux-raid
In-Reply-To: <ECED3C91-8D12-469E-A9AB-1337292DA216@aol.com>
On Tue, Mar 08, 2016 at 10:20:17PM -0500, Dan Russell wrote:
> The partially-rebuilt drive is sdk, the original “failed” drive is sdag
Best to leave both out if one has outdated and the other only half
the content...
> However fdisk and mdadm are reporting the array is 17.6TB in size, whereas it should be 66TB (24 3TB HDDs RAID6).
I reproduced your commands using tmpfs based loop devices and it gives me
the same problem. The RAID size is only 16 TiB. It seems to be hitting a
limit somewhere.
Your /dev/mapper/sdx are snapshot/overlays, I hope?
DDF metadata seems to be located at the end of the device, so you could try
your luck with mdadm 1.0 metadata instead; that gives me a RAID of a size
closer to home.
# mdadm --create /dev/md42 --assume-clean --metadata=1.0 --level=6 --raid-devices=24 --chunk=64 /dev/loop{0..23}
# fdisk -l /dev/md42
Disk /dev/md42: 60 TiB, 65999996846080 bytes, 128906243840 sectors
Units: sectors of 1 * 512 = 512 bytes
Sector size (logical/physical): 512 bytes / 512 bytes
I/O size (minimum/optimal): 65536 bytes / 1441792 bytes
It wastes some sectors at the end though, not sure if more or less
than what DDF uses for metadata. You might have to add some empty
space to your device mappings to get a full view.
Regards
Andreas Klauer
--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] md/raid5: preserve STRIPE_PREREAD_ACTIVE in break_stripe_batch_list
From: Shaohua Li @ 2016-03-09 19:23 UTC (permalink / raw)
To: NeilBrown; +Cc: Shaohua Li, linux-raid, LKML, Martin Svec
In-Reply-To: <87lh5rjw6p.fsf@notabene.neil.brown.name>
On Thu, Mar 10, 2016 at 06:19:42AM +1100, Neil Brown wrote:
> On Thu, Mar 10 2016, Shaohua Li wrote:
>
> > On Wed, Mar 09, 2016 at 12:58:25PM +1100, Neil Brown wrote:
> >>
> >> break_stripe_batch_list breaks up a batch and copies some flags from
> >> the batch head to the members, preserving others.
> >>
> >> It doesn't preserve or copy STRIPE_PREREAD_ACTIVE. This is not
> >> normally a problem as STRIPE_PREREAD_ACTIVE is cleared when a
> >> stripe_head is added to a batch, and is not set on stripe_heads
> >> already in a batch.
> >>
> >> However there is no locking to ensure one thread doesn't set the flag
> >> after it has just been cleared in another. This does occasionally happen.
> >>
> >> md/raid5 maintains a count of the number of stripe_heads with
> >> STRIPE_PREREAD_ACTIVE set: conf->preread_active_stripes. When
> >> break_stripe_batch_list clears STRIPE_PREREAD_ACTIVE inadvertently
> >> this could becomes incorrect and will never again return to zero.
> >>
> >> md/raid5 delays the handling of some stripe_heads until
> >> preread_active_stripes becomes zero. So when the above mention race
> >> happens, those stripe_heads become blocked and never progress,
> >> resulting is write to the array handing.
> >>
> >> So: change break_stripe_batch_list to preserve STRIPE_PREREAD_ACTIVE
> >> in the members of a batch.
> >>
> >> URL: https://bugzilla.kernel.org/show_bug.cgi?id=108741
> >> URL: https://bugzilla.redhat.com/show_bug.cgi?id=1258153
> >> URL: http://thread.gmane.org/5649C0E9.2030204@zoner.cz
> >> Reported-by: Martin Svec <martin.svec@zoner.cz> (and others)
> >> Tested-by: Tom Weber <linux@junkyard.4t2.com>
> >> Fixes: 1b956f7a8f9a ("md/raid5: be more selective about distributing flags across batch.")
> >> Cc: stable@vger.kernel.org (v4.1 and later)
> >> Signed-off-by: NeilBrown <neilb@suse.com>
> >
> > Applied, thanks Neil! I'll split the WARN_ON_ONCE and do it for each bit, so
> > next time we can have clear clue.
>
> I personally think that would look ugly and increase the in-line code
> size for minimal gain.
> If you want to make a change (which I'm in two minds about) I think it
> would be much cleaner to do
> if (WARN_ON_ONCE(...)) printk(....);
>
> Then at least the extra code will be out of line - not even loaded into
> the instruction cache until needed.
There is a handy WARN_ONCE(). It's like WARN_ON_ONCE() but allows printing exra info.
Thanks,
Shaohua
^ permalink raw reply
* Re: [PATCH] md/raid5: preserve STRIPE_PREREAD_ACTIVE in break_stripe_batch_list
From: NeilBrown @ 2016-03-09 19:19 UTC (permalink / raw)
To: Shaohua Li; +Cc: Shaohua Li, linux-raid, LKML, Martin Svec
In-Reply-To: <20160309172652.GA8073@kernel.org>
[-- Attachment #1: Type: text/plain, Size: 3273 bytes --]
On Thu, Mar 10 2016, Shaohua Li wrote:
> On Wed, Mar 09, 2016 at 12:58:25PM +1100, Neil Brown wrote:
>>
>> break_stripe_batch_list breaks up a batch and copies some flags from
>> the batch head to the members, preserving others.
>>
>> It doesn't preserve or copy STRIPE_PREREAD_ACTIVE. This is not
>> normally a problem as STRIPE_PREREAD_ACTIVE is cleared when a
>> stripe_head is added to a batch, and is not set on stripe_heads
>> already in a batch.
>>
>> However there is no locking to ensure one thread doesn't set the flag
>> after it has just been cleared in another. This does occasionally happen.
>>
>> md/raid5 maintains a count of the number of stripe_heads with
>> STRIPE_PREREAD_ACTIVE set: conf->preread_active_stripes. When
>> break_stripe_batch_list clears STRIPE_PREREAD_ACTIVE inadvertently
>> this could becomes incorrect and will never again return to zero.
>>
>> md/raid5 delays the handling of some stripe_heads until
>> preread_active_stripes becomes zero. So when the above mention race
>> happens, those stripe_heads become blocked and never progress,
>> resulting is write to the array handing.
>>
>> So: change break_stripe_batch_list to preserve STRIPE_PREREAD_ACTIVE
>> in the members of a batch.
>>
>> URL: https://bugzilla.kernel.org/show_bug.cgi?id=108741
>> URL: https://bugzilla.redhat.com/show_bug.cgi?id=1258153
>> URL: http://thread.gmane.org/5649C0E9.2030204@zoner.cz
>> Reported-by: Martin Svec <martin.svec@zoner.cz> (and others)
>> Tested-by: Tom Weber <linux@junkyard.4t2.com>
>> Fixes: 1b956f7a8f9a ("md/raid5: be more selective about distributing flags across batch.")
>> Cc: stable@vger.kernel.org (v4.1 and later)
>> Signed-off-by: NeilBrown <neilb@suse.com>
>
> Applied, thanks Neil! I'll split the WARN_ON_ONCE and do it for each bit, so
> next time we can have clear clue.
I personally think that would look ugly and increase the in-line code
size for minimal gain.
If you want to make a change (which I'm in two minds about) I think it
would be much cleaner to do
if (WARN_ON_ONCE(...)) printk(....);
Then at least the extra code will be out of line - not even loaded into
the instruction cache until needed.
Thanks,
NeilBrown
>
> Thanks,
> Shaohua
>
>> ---
>> drivers/md/raid5.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
>> index b4f02c9959f2..2e7d253be6ce 100644
>> --- a/drivers/md/raid5.c
>> +++ b/drivers/md/raid5.c
>> @@ -4236,7 +4236,6 @@ static void break_stripe_batch_list(struct stripe_head *head_sh,
>> WARN_ON_ONCE(sh->state & ((1 << STRIPE_ACTIVE) |
>> (1 << STRIPE_SYNCING) |
>> (1 << STRIPE_REPLACED) |
>> - (1 << STRIPE_PREREAD_ACTIVE) |
>> (1 << STRIPE_DELAYED) |
>> (1 << STRIPE_BIT_DELAY) |
>> (1 << STRIPE_FULL_WRITE) |
>> @@ -4251,6 +4250,7 @@ static void break_stripe_batch_list(struct stripe_head *head_sh,
>> (1 << STRIPE_REPLACED)));
>>
>> set_mask_bits(&sh->state, ~(STRIPE_EXPAND_SYNC_FLAGS |
>> + (1 << STRIPE_PREREAD_ACTIVE) |
>> (1 << STRIPE_DEGRADED)),
>> head_sh->state & (1 << STRIPE_INSYNC));
>>
>> --
>> 2.7.2
>>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]
^ permalink raw reply
* Re: [PATCH] Fix regression during add devices
From: Jes Sorensen @ 2016-03-09 18:30 UTC (permalink / raw)
To: Coly Li; +Cc: linux-raid, Hannes Reinecke, Neil Brown
In-Reply-To: <1457500818-1586-1-git-send-email-colyli@suse.de>
Coly Li <colyli@suse.de> writes:
> From: Hannes Reinecke <hare@suse.de>
>
> Commit d180d2aa2a17 ("Manage: fix test for 'is array failed'.")
> introduced a regression which would not allow to re-add new
> drivers to a failed array.
>
> The patch is written by Hannes Reinecke, Neil Brown points out
> the buggy commit ID is d180d2aa2a17. Coly helps to submit the
> patch to mdadm upstream.
>
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> Cc: Coly Li <colyli@suse.de>
> Cc: Neil Brown <neilb@suse.com>
> ---
> Manage.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Coly,
Thanks for the fix - please use "Fixes: " to indicate which patch
introduced the bug, and the story of pushing the patch really belongs in
a comment below the --- line, so it doesn't go into git.
I modified it to look like this - please let me know if you are OK with
this.
Thanks,
Jes
commit 227bd322d3ed80bee0f1a0ae44a293908890851c
Author: Hannes Reinecke <hare@suse.de>
Date: Wed Mar 9 13:20:18 2016 +0800
Fix regression during add devices
Commit d180d2aa2a17 ("Manage: fix test for 'is array failed'.")
introduced a regression which would not allow to re-add new
drivers to a failed array.
Fixes: d180d2aa2a17 ("Manage: fix test for 'is array failed'.")
Signed-off-by: Hannes Reinecke <hare@suse.de>
Cc: Coly Li <colyli@suse.de>
Cc: Neil Brown <neilb@suse.com>
Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
> diff --git a/Manage.c b/Manage.c
> index 7e1b94b..073ddb9 100644
> --- a/Manage.c
> +++ b/Manage.c
> @@ -879,10 +879,10 @@ int Manage_add(int fd, int tfd, struct mddev_dev *dv,
> continue;
> if (disc.major == 0 && disc.minor == 0)
> continue;
> - found++;
> if (!(disc.state & (1<<MD_DISK_SYNC)))
> continue;
> avail[disc.raid_disk] = 1;
> + found++;
> }
> array_failed = !enough(array->level, array->raid_disks,
> array->layout, 1, avail);
^ permalink raw reply
* Re: [PATCH 4/8] Grow: Grow_addbitmap(): Add check to quiet down static code checkers
From: Guoqing Jiang @ 2016-03-09 17:42 UTC (permalink / raw)
To: Jes.Sorensen, linux-raid; +Cc: neilb, pawel.baldysiak
In-Reply-To: <1457458252-20203-5-git-send-email-Jes.Sorensen@redhat.com>
On 03/09/2016 01:30 AM, Jes.Sorensen@redhat.com wrote:
> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>
> Grow_addbitmap() is only ever called with s->bitmap_file != NULL, but
> not all static code checkers catch this. This adds a check to quiet
> down the false positive warnings.
>
> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
> ---
> Grow.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/Grow.c b/Grow.c
> index 0fa776d..c453eb6 100755
> --- a/Grow.c
> +++ b/Grow.c
> @@ -297,7 +297,14 @@ int Grow_addbitmap(char *devname, int fd, struct context *c, struct shape *s)
> " between different architectures. Consider upgrading the Linux kernel.\n");
> }
>
> - if (s->bitmap_file && strcmp(s->bitmap_file, "clustered") == 0)
> + /*
> + * We only ever get called if s->bitmap_file is != NULL, so this check
> + * is just here to quiet down static code checkers.
> + */
> + if (!s->bitmap_file)
> + return 1;
Is it really need to make all static code checkers happy? ;-)
Otherwise, I would prefer remove above check.
Anyway, I am fine with the changes.
Thanks,
Guoqing
> +
> + if (strcmp(s->bitmap_file, "clustered") == 0)
> major = BITMAP_MAJOR_CLUSTERED;
>
> if (ioctl(fd, GET_BITMAP_FILE, &bmf) != 0) {
^ permalink raw reply
* Re: [PATCH] md/raid5: preserve STRIPE_PREREAD_ACTIVE in break_stripe_batch_list
From: Shaohua Li @ 2016-03-09 17:26 UTC (permalink / raw)
To: NeilBrown; +Cc: Shaohua Li, linux-raid, LKML, Martin Svec
In-Reply-To: <87r3fkjttq.fsf@notabene.neil.brown.name>
On Wed, Mar 09, 2016 at 12:58:25PM +1100, Neil Brown wrote:
>
> break_stripe_batch_list breaks up a batch and copies some flags from
> the batch head to the members, preserving others.
>
> It doesn't preserve or copy STRIPE_PREREAD_ACTIVE. This is not
> normally a problem as STRIPE_PREREAD_ACTIVE is cleared when a
> stripe_head is added to a batch, and is not set on stripe_heads
> already in a batch.
>
> However there is no locking to ensure one thread doesn't set the flag
> after it has just been cleared in another. This does occasionally happen.
>
> md/raid5 maintains a count of the number of stripe_heads with
> STRIPE_PREREAD_ACTIVE set: conf->preread_active_stripes. When
> break_stripe_batch_list clears STRIPE_PREREAD_ACTIVE inadvertently
> this could becomes incorrect and will never again return to zero.
>
> md/raid5 delays the handling of some stripe_heads until
> preread_active_stripes becomes zero. So when the above mention race
> happens, those stripe_heads become blocked and never progress,
> resulting is write to the array handing.
>
> So: change break_stripe_batch_list to preserve STRIPE_PREREAD_ACTIVE
> in the members of a batch.
>
> URL: https://bugzilla.kernel.org/show_bug.cgi?id=108741
> URL: https://bugzilla.redhat.com/show_bug.cgi?id=1258153
> URL: http://thread.gmane.org/5649C0E9.2030204@zoner.cz
> Reported-by: Martin Svec <martin.svec@zoner.cz> (and others)
> Tested-by: Tom Weber <linux@junkyard.4t2.com>
> Fixes: 1b956f7a8f9a ("md/raid5: be more selective about distributing flags across batch.")
> Cc: stable@vger.kernel.org (v4.1 and later)
> Signed-off-by: NeilBrown <neilb@suse.com>
Applied, thanks Neil! I'll split the WARN_ON_ONCE and do it for each bit, so
next time we can have clear clue.
Thanks,
Shaohua
> ---
> drivers/md/raid5.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index b4f02c9959f2..2e7d253be6ce 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -4236,7 +4236,6 @@ static void break_stripe_batch_list(struct stripe_head *head_sh,
> WARN_ON_ONCE(sh->state & ((1 << STRIPE_ACTIVE) |
> (1 << STRIPE_SYNCING) |
> (1 << STRIPE_REPLACED) |
> - (1 << STRIPE_PREREAD_ACTIVE) |
> (1 << STRIPE_DELAYED) |
> (1 << STRIPE_BIT_DELAY) |
> (1 << STRIPE_FULL_WRITE) |
> @@ -4251,6 +4250,7 @@ static void break_stripe_batch_list(struct stripe_head *head_sh,
> (1 << STRIPE_REPLACED)));
>
> set_mask_bits(&sh->state, ~(STRIPE_EXPAND_SYNC_FLAGS |
> + (1 << STRIPE_PREREAD_ACTIVE) |
> (1 << STRIPE_DEGRADED)),
> head_sh->state & (1 << STRIPE_INSYNC));
>
> --
> 2.7.2
>
^ permalink raw reply
* Re: [PATCH RESEND v2 16/18] fuse: Support fuse filesystems outside of init_user_ns
From: Seth Forshee @ 2016-03-09 17:07 UTC (permalink / raw)
To: Miklos Szeredi
Cc: linux-bcache-u79uwXL29TY76Z2rM5mHXA, Serge Hallyn,
dm-devel-H+wXaHxf7aLQT0dZR+AlfA, LSM, Kernel Mailing List,
linux-raid-u79uwXL29TY76Z2rM5mHXA, fuse-devel,
Austin S Hemmelgarn, linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
Eric W. Biederman, selinux-+05T5uksL2qpZYMLLGbcSA, Linux-Fsdevel,
Alexander Viro
In-Reply-To: <CAJfpegv5KR_Hi-79a8oyb+R+tv9W3RYqy5pngUKSyauVNk2ScQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Wed, Mar 09, 2016 at 04:51:42PM +0100, Miklos Szeredi wrote:
> On Wed, Mar 9, 2016 at 4:25 PM, Seth Forshee <seth.forshee-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org> wrote:
> > On Wed, Mar 09, 2016 at 03:48:22PM +0100, Miklos Szeredi wrote:
>
> >> Can't we use current_cred()->uid/gid? Or fsuid/fsgid maybe?
> >
> > That would be a departure from the current behavior in the !allow_other
> > case for unprivileged users. Since those mounts are done by an suid
> > helper all of those ids would be root in the userns, wouldn't they?
>
> Well, actually this is what the helper does:
>
> sprintf(d, "fd=%i,rootmode=%o,user_id=%u,group_id=%u",
> fd, rootmode, getuid(), getgid());
Sorry, I was thinking of euid. So this may not be a problem.
> So it just uses the current uid/gid. Apparently no reason to do this
> in userland, we could just as well set these in the kernel. Except
> for possible backward compatibility problems for things not using the
> helper.
>
> BUT if the mount is unprivileged or it's a userns mount, or anything
> previously not possible, then we are not constrained by the backward
> compatibility issues, and can go with the saner solution.
>
> Does that not make sense?
But we generally do want backwards compatibility, and we want userspace
software to be able to expect the same behavior whether or not it's
running in a user namespaced container. Obviously we can't always have
things 100% identical, but we shouldn't break things unless we really
need to.
However it may be that this isn't actually going to break assumptions of
existing software like I had feared. My preference is still to not
change any userspace-visible behaviors since we never know what software
might have made assumptions based on those behaviors. But if you're
confident that it won't break anything I'm willing to give it a try.
> >> When we have true unprivileged mounts, the user_id/group_id options
> >> become redundant anyway and we can just use the current credentials.
> >
> > True, but we don't yet have that.
>
> What's missing?
A user must still be privileged to mount, even if only towards their own
user and mount namespaces. Maybe that's what you meant though and I just
misunderstood.
Thanks,
Seth
------------------------------------------------------------------------------
Transform Data into Opportunity.
Accelerate data analysis in your applications with
Intel Data Analytics Acceleration Library.
Click to learn more.
http://pubads.g.doubleclick.net/gampad/clk?id=278785111&iu=/4140
--
fuse-devel mailing list
To unsubscribe or subscribe, visit https://lists.sourceforge.net/lists/listinfo/fuse-devel
^ permalink raw reply
* Re: [PATCH 0/8] mdadm static checker fixes
From: Jes Sorensen @ 2016-03-09 16:30 UTC (permalink / raw)
To: NeilBrown; +Cc: linux-raid, gqjiang, pawel.baldysiak
In-Reply-To: <877fhclgvn.fsf@notabene.neil.brown.name>
NeilBrown <neilb@suse.de> writes:
> On Wed, Mar 09 2016, Jes.Sorensen@redhat.com wrote:
>
>> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>>
>> Hi,
>>
>> I have been running mdadm through Coverity and fixed a number of
>> issues that were raised in the scan. A couple of them were non issues
>> related to conditions where we know for sure the kernel will not
>> return strings longer than a given size, but there were also a number
>> of potential memory leaks and buffer overflows.
>>
>> These patches are sitting in my pending queue. If you are on the CC
>> list, would you mind having a look at the portions touching code you
>> previously pushed wrote.
>>
>> Please hollor if you notice I did anything wrong, otherwise I'll push
>> this set into git within the next couple of days.
>
> All
> Reviewed-by: NeilBrown <neilb@suse.com>
>
> The comments I have made are only possible enhancements, no problems
> found.
>
> I must confess that I was generally fairly careless about resource
> leakage. mdadm usually calls 'exit' fairly soon and that releases
> everything. But mdmon and "mdadm --monitor" at long-running so it can
> pay to be careful. And I'm very supportive of silencing warnings from
> tools that also provide useful warnings.
Thanks for the feedback - much appreciated!
Cheers,
Jes
> Thanks!
>
> NeilBrown
>
>>
>> Cheers,
>> Jes
>>
>>
>> Jes Sorensen (8):
>> Manage: Manage_add(): Fix memory leak
>> load_sys(): Add a buffer size argument
>> Grow: Grow_continue_command() remove dead code
>> Grow: Grow_addbitmap(): Add check to quiet down static code checkers
>> {platform,super}-intel: Fix two resource leaks
>> bitmap: Fix resource leak in bitmap_file_open()
>> Manage: Manage_subdevs() fix file descriptor leak
>> super1: Fix potential buffer overflows when copying cluster_name
>>
>> Detail.c | 2 +-
>> Grow.c | 11 ++++++++---
>> Manage.c | 8 ++++++--
>> bitmap.c | 1 +
>> mdadm.h | 2 +-
>> platform-intel.c | 7 ++++++-
>> super-intel.c | 12 ++++++++----
>> super1.c | 18 ++++++++++++------
>> sysfs.c | 47 ++++++++++++++++++++++++-----------------------
>> 9 files changed, 67 insertions(+), 41 deletions(-)
>>
>> --
>> 2.5.0
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-raid" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 6/8] bitmap: Fix resource leak in bitmap_file_open()
From: Jes Sorensen @ 2016-03-09 16:28 UTC (permalink / raw)
To: NeilBrown; +Cc: linux-raid, gqjiang, pawel.baldysiak
In-Reply-To: <87a8m8lh2t.fsf@notabene.neil.brown.name>
NeilBrown <neilb@suse.de> writes:
> On Wed, Mar 09 2016, Jes.Sorensen@redhat.com wrote:
>
>> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>>
>> The code would leak 'fd' if locate_bitmap() failed.
>>
>> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
>> ---
>> bitmap.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/bitmap.c b/bitmap.c
>> index 5ad7401..0367d13 100644
>> --- a/bitmap.c
>> +++ b/bitmap.c
>> @@ -224,6 +224,7 @@ int bitmap_file_open(char *filename, struct supertype **stp, int node_num)
>> } else {
>> if (st->ss->locate_bitmap(st, fd, node_num)) {
>> pr_err("%s doesn't have bitmap\n", filename);
>> + close(fd);
>> fd = -1;
>> }
>> }
>
> Don't you also need a 'close' in
>
> } else if (!st->ss->locate_bitmap) {
> pr_err("No bitmap possible with %s metadata\n",
> st->ss->name);
> return -1;
> ??
I probably need new glasses too :)
Thanks for catching that!
Cheers,
Jes
^ permalink raw reply
* Re: [PATCH 5/8] {platform,super}-intel: Fix two resource leaks
From: Jes Sorensen @ 2016-03-09 16:23 UTC (permalink / raw)
To: NeilBrown; +Cc: linux-raid, gqjiang, pawel.baldysiak
In-Reply-To: <87d1r4lhb8.fsf@notabene.neil.brown.name>
NeilBrown <neilb@suse.de> writes:
> On Wed, Mar 09 2016, Jes.Sorensen@redhat.com wrote:
>
>> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>>
>> The code did not free 'dir' allocated by opendir(). An additional
>> benefit is that this simplifies the for() loops.
>>
>> Fixes: 60f0f54d ("IMSM: Add support for VMD")
>> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
>> ---
>> platform-intel.c | 7 ++++++-
>> super-intel.c | 6 +++++-
>> 2 files changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/platform-intel.c b/platform-intel.c
>> index 88818f3..c60fd9e 100644
>> --- a/platform-intel.c
>> +++ b/platform-intel.c
>> @@ -724,8 +724,10 @@ char *vmd_domain_to_controller(struct sys_dev *hba, char *buf)
>> return NULL;
>>
>> dir = opendir("/sys/bus/pci/drivers/vmd");
>> + if (!dir)
>> + return NULL;
>>
>> - for (ent = dir ? readdir(dir) : NULL; ent; ent = readdir(dir)) {
>> + for (ent = readdir(dir); ent; ent = readdir(dir)) {
>> sprintf(path, "/sys/bus/pci/drivers/vmd/%s/domain/device",
>> ent->d_name);
>>
>> @@ -734,8 +736,11 @@ char *vmd_domain_to_controller(struct sys_dev *hba, char *buf)
>>
>> if (strncmp(buf, hba->path, strlen(buf)) == 0) {
>> sprintf(path, "/sys/bus/pci/drivers/vmd/%s", ent->d_name);
>> + closedir(dir);
>> return realpath(path, buf);
>> }
>> }
>> +
>> + closedir(dir);
>> return NULL;
>> }
>> diff --git a/super-intel.c b/super-intel.c
>> index 158f4e8..e1bee75 100644
>> --- a/super-intel.c
>> +++ b/super-intel.c
>> @@ -1781,7 +1781,10 @@ static int print_vmd_attached_devs(struct sys_dev *hba)
>> * this hba
>> */
>> dir = opendir("/sys/bus/pci/drivers/nvme");
>> - for (ent = dir ? readdir(dir) : NULL; ent; ent = readdir(dir)) {
>> + if (!dir)
>> + return 1;
>> +
>
> Returning '1' looks really weird here. I can see it is consistent with
> if (hba->type != SYS_DEV_VMD)
> return 1;
>
> above, but still....
> As the return value is never used, should we just make it 'void' ??
Seems reasonable - I'll put that in a separate patch.
Cheers,
Jes
^ permalink raw reply
* Re: [PATCH 3/8] Grow: Grow_continue_command() remove dead code
From: Jes Sorensen @ 2016-03-09 16:19 UTC (permalink / raw)
To: NeilBrown; +Cc: linux-raid, gqjiang, pawel.baldysiak
In-Reply-To: <87fuw0lhhf.fsf@notabene.neil.brown.name>
NeilBrown <neilb@suse.de> writes:
> On Wed, Mar 09 2016, Jes.Sorensen@redhat.com wrote:
>
>> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>>
>> All cases where fd2 is used are completed with a close(fd2) ; fd2 = -1;
>> so there is no need to check for fd2 > -1 before exiting.
>
> In that case, you can remove all the assignments so -1 to fd2 - if you
> like.
Oh good point - I'll make that change.
Thanks,
Jes
>
> Thanks,
> NeilBrown
>
>>
>> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
>> ---
>> Grow.c | 2 --
>> 1 file changed, 2 deletions(-)
>>
>> diff --git a/Grow.c b/Grow.c
>> index c4f417e..0fa776d 100755
>> --- a/Grow.c
>> +++ b/Grow.c
>> @@ -4924,8 +4924,6 @@ int Grow_continue_command(char *devname, int fd,
>> ret_val = Grow_continue(fd, st, content, backup_file, 1, 0);
>>
>> Grow_continue_command_exit:
>> - if (fd2 > -1)
>> - close(fd2);
>> if (cfd > -1)
>> close(cfd);
>> st->ss->free_super(st);
>> --
>> 2.5.0
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-raid" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH RESEND v2 16/18] fuse: Support fuse filesystems outside of init_user_ns
From: Miklos Szeredi @ 2016-03-09 15:51 UTC (permalink / raw)
To: Seth Forshee
Cc: Eric W. Biederman, Alexander Viro, Serge Hallyn,
Richard Weinberger, Austin S Hemmelgarn, Kernel Mailing List,
linux-bcache, dm-devel, linux-raid, linux-mtd, Linux-Fsdevel,
fuse-devel, LSM, selinux
In-Reply-To: <20160309152505.GA28779@ubuntu-hedt>
On Wed, Mar 9, 2016 at 4:25 PM, Seth Forshee <seth.forshee@canonical.com> wrote:
> On Wed, Mar 09, 2016 at 03:48:22PM +0100, Miklos Szeredi wrote:
>> Can't we use current_cred()->uid/gid? Or fsuid/fsgid maybe?
>
> That would be a departure from the current behavior in the !allow_other
> case for unprivileged users. Since those mounts are done by an suid
> helper all of those ids would be root in the userns, wouldn't they?
Well, actually this is what the helper does:
sprintf(d, "fd=%i,rootmode=%o,user_id=%u,group_id=%u",
fd, rootmode, getuid(), getgid());
So it just uses the current uid/gid. Apparently no reason to do this
in userland, we could just as well set these in the kernel. Except
for possible backward compatibility problems for things not using the
helper.
BUT if the mount is unprivileged or it's a userns mount, or anything
previously not possible, then we are not constrained by the backward
compatibility issues, and can go with the saner solution.
Does that not make sense?
>> When we have true unprivileged mounts, the user_id/group_id options
>> become redundant anyway and we can just use the current credentials.
>
> True, but we don't yet have that.
What's missing?
Thanks,
Miklos
^ permalink raw reply
* Re: [PATCH RESEND v2 16/18] fuse: Support fuse filesystems outside of init_user_ns
From: Seth Forshee @ 2016-03-09 15:25 UTC (permalink / raw)
To: Miklos Szeredi
Cc: linux-bcache-u79uwXL29TY76Z2rM5mHXA, Serge Hallyn,
dm-devel-H+wXaHxf7aLQT0dZR+AlfA, LSM, Kernel Mailing List,
linux-raid-u79uwXL29TY76Z2rM5mHXA, fuse-devel,
Austin S Hemmelgarn, linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
Eric W. Biederman, selinux-+05T5uksL2qpZYMLLGbcSA, Linux-Fsdevel,
Alexander Viro
In-Reply-To: <CAJfpegv5JmB15yHpjYxVeOYdWWkoLMftr9-e_iS93Y_7m=t4Zw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Wed, Mar 09, 2016 at 03:48:22PM +0100, Miklos Szeredi wrote:
> On Wed, Mar 9, 2016 at 3:18 PM, Seth Forshee <seth.forshee-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org> wrote:
> > On Wed, Mar 09, 2016 at 12:29:23PM +0100, Miklos Szeredi wrote:
> >> On Mon, Jan 04, 2016 at 12:03:55PM -0600, Seth Forshee wrote:
>
> >> > -static int parse_fuse_opt(char *opt, struct fuse_mount_data *d, int is_bdev)
> >> > +static int parse_fuse_opt(char *opt, struct fuse_mount_data *d, int is_bdev,
> >> > + struct user_namespace *user_ns)
> >> > {
> >> > char *p;
> >> > memset(d, 0, sizeof(struct fuse_mount_data));
> >> > d->max_read = ~0;
> >> > d->blksize = FUSE_DEFAULT_BLKSIZE;
> >> > + d->user_id = make_kuid(user_ns, 0);
> >> > + d->group_id = make_kgid(user_ns, 0);
> >>
> >> It is true that if "user_id=" or "group_id" options were omitted we used the
> >> zero uid/gid values. However, this isn't actually used by anybody AFAIK, and
> >> generalizing it for userns doesn't seem to make much sense.
> >>
> >> So I suggest we that we instead return an error if mounting from a userns AND
> >> neither "allow_other" nor both "user_id" and "group_id" are specified.
> >
> > But those are also used for ownership of the connection files in
> > fusectl. In an allow_other mount shouldn't those files by owned by
> > namespace root and not global root?
>
> Yes.
>
> Can't we use current_cred()->uid/gid? Or fsuid/fsgid maybe?
That would be a departure from the current behavior in the !allow_other
case for unprivileged users. Since those mounts are done by an suid
helper all of those ids would be root in the userns, wouldn't they?
> When we have true unprivileged mounts, the user_id/group_id options
> become redundant anyway and we can just use the current credentials.
True, but we don't yet have that.
Thanks,
Seth
------------------------------------------------------------------------------
Transform Data into Opportunity.
Accelerate data analysis in your applications with
Intel Data Analytics Acceleration Library.
Click to learn more.
http://pubads.g.doubleclick.net/gampad/clk?id=278785111&iu=/4140
--
fuse-devel mailing list
To unsubscribe or subscribe, visit https://lists.sourceforge.net/lists/listinfo/fuse-devel
^ permalink raw reply
* Re: [PATCH RESEND v2 16/18] fuse: Support fuse filesystems outside of init_user_ns
From: Miklos Szeredi @ 2016-03-09 14:48 UTC (permalink / raw)
To: Seth Forshee
Cc: linux-bcache-u79uwXL29TY76Z2rM5mHXA, Serge Hallyn,
dm-devel-H+wXaHxf7aLQT0dZR+AlfA, LSM, Kernel Mailing List,
linux-raid-u79uwXL29TY76Z2rM5mHXA, fuse-devel,
Austin S Hemmelgarn, linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
Eric W. Biederman, selinux-+05T5uksL2qpZYMLLGbcSA, Linux-Fsdevel,
Alexander Viro
In-Reply-To: <20160309141840.GC23399@ubuntu-xps13>
On Wed, Mar 9, 2016 at 3:18 PM, Seth Forshee <seth.forshee-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org> wrote:
> On Wed, Mar 09, 2016 at 12:29:23PM +0100, Miklos Szeredi wrote:
>> On Mon, Jan 04, 2016 at 12:03:55PM -0600, Seth Forshee wrote:
>> > -static int parse_fuse_opt(char *opt, struct fuse_mount_data *d, int is_bdev)
>> > +static int parse_fuse_opt(char *opt, struct fuse_mount_data *d, int is_bdev,
>> > + struct user_namespace *user_ns)
>> > {
>> > char *p;
>> > memset(d, 0, sizeof(struct fuse_mount_data));
>> > d->max_read = ~0;
>> > d->blksize = FUSE_DEFAULT_BLKSIZE;
>> > + d->user_id = make_kuid(user_ns, 0);
>> > + d->group_id = make_kgid(user_ns, 0);
>>
>> It is true that if "user_id=" or "group_id" options were omitted we used the
>> zero uid/gid values. However, this isn't actually used by anybody AFAIK, and
>> generalizing it for userns doesn't seem to make much sense.
>>
>> So I suggest we that we instead return an error if mounting from a userns AND
>> neither "allow_other" nor both "user_id" and "group_id" are specified.
>
> But those are also used for ownership of the connection files in
> fusectl. In an allow_other mount shouldn't those files by owned by
> namespace root and not global root?
Yes.
Can't we use current_cred()->uid/gid? Or fsuid/fsgid maybe?
When we have true unprivileged mounts, the user_id/group_id options
become redundant anyway and we can just use the current credentials.
Thanks,
Miklos
------------------------------------------------------------------------------
Transform Data into Opportunity.
Accelerate data analysis in your applications with
Intel Data Analytics Acceleration Library.
Click to learn more.
http://pubads.g.doubleclick.net/gampad/clk?id=278785111&iu=/4140
--
fuse-devel mailing list
To unsubscribe or subscribe, visit https://lists.sourceforge.net/lists/listinfo/fuse-devel
^ permalink raw reply
* Re: [PATCH RESEND v2 16/18] fuse: Support fuse filesystems outside of init_user_ns
From: Seth Forshee @ 2016-03-09 14:18 UTC (permalink / raw)
To: Miklos Szeredi
Cc: Eric W. Biederman, Alexander Viro, Serge Hallyn,
Richard Weinberger, Austin S Hemmelgarn, linux-kernel,
linux-bcache, dm-devel, linux-raid, linux-mtd, linux-fsdevel,
fuse-devel, linux-security-module, selinux
In-Reply-To: <20160309112923.GF8655@tucsk>
On Wed, Mar 09, 2016 at 12:29:23PM +0100, Miklos Szeredi wrote:
> On Mon, Jan 04, 2016 at 12:03:55PM -0600, Seth Forshee wrote:
> > In order to support mounts from namespaces other than
> > init_user_ns, fuse must translate uids and gids to/from the
> > userns of the process servicing requests on /dev/fuse. This
> > patch does that, with a couple of restrictions on the namespace:
> >
> > - The userns for the fuse connection is fixed to the namespace
> > from which /dev/fuse is opened.
> >
> > - The namespace must be the same as s_user_ns.
> >
> > These restrictions simplify the implementation by avoiding the
> > need to pass around userns references and by allowing fuse to
> > rely on the checks in inode_change_ok for ownership changes.
> > Either restriction could be relaxed in the future if needed.
> >
> > For cuse the namespace used for the connection is also simply
> > current_user_ns() at the time /dev/cuse is opened.
> >
> > Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
> > ---
> > fs/fuse/cuse.c | 3 ++-
> > fs/fuse/dev.c | 13 ++++++++-----
> > fs/fuse/dir.c | 14 +++++++-------
> > fs/fuse/fuse_i.h | 6 +++++-
> > fs/fuse/inode.c | 35 +++++++++++++++++++++++------------
> > 5 files changed, 45 insertions(+), 26 deletions(-)
> >
> > diff --git a/fs/fuse/cuse.c b/fs/fuse/cuse.c
> > index eae2c11268bc..a10aca57bfe4 100644
> > --- a/fs/fuse/cuse.c
> > +++ b/fs/fuse/cuse.c
> > @@ -48,6 +48,7 @@
> > #include <linux/stat.h>
> > #include <linux/module.h>
> > #include <linux/uio.h>
> > +#include <linux/user_namespace.h>
> >
> > #include "fuse_i.h"
> >
> > @@ -498,7 +499,7 @@ static int cuse_channel_open(struct inode *inode, struct file *file)
> > if (!cc)
> > return -ENOMEM;
> >
> > - fuse_conn_init(&cc->fc);
> > + fuse_conn_init(&cc->fc, current_user_ns());
> >
> > fud = fuse_dev_alloc(&cc->fc);
> > if (!fud) {
> > diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
> > index a4f6f30d6d86..11b4cb0a0e2f 100644
> > --- a/fs/fuse/dev.c
> > +++ b/fs/fuse/dev.c
> > @@ -127,8 +127,8 @@ static void __fuse_put_request(struct fuse_req *req)
> >
> > static void fuse_req_init_context(struct fuse_conn *fc, struct fuse_req *req)
> > {
> > - req->in.h.uid = from_kuid_munged(&init_user_ns, current_fsuid());
> > - req->in.h.gid = from_kgid_munged(&init_user_ns, current_fsgid());
> > + req->in.h.uid = from_kuid(fc->user_ns, current_fsuid());
> > + req->in.h.gid = from_kgid(fc->user_ns, current_fsgid());
> > req->in.h.pid = pid_nr_ns(task_pid(current), fc->pid_ns);
> > }
> >
> > @@ -186,7 +186,8 @@ static struct fuse_req *__fuse_get_req(struct fuse_conn *fc, unsigned npages,
> > __set_bit(FR_WAITING, &req->flags);
> > if (for_background)
> > __set_bit(FR_BACKGROUND, &req->flags);
> > - if (req->in.h.pid == 0) {
> > + if (req->in.h.pid == 0 || req->in.h.uid == (uid_t)-1 ||
> > + req->in.h.gid == (gid_t)-1) {
> > fuse_put_request(fc, req);
> > return ERR_PTR(-EOVERFLOW);
> > }
> > @@ -1248,7 +1249,8 @@ static ssize_t fuse_dev_do_read(struct fuse_dev *fud, struct file *file,
> > struct fuse_in *in;
> > unsigned reqsize;
> >
> > - if (task_active_pid_ns(current) != fc->pid_ns)
> > + if (task_active_pid_ns(current) != fc->pid_ns ||
> > + current_user_ns() != fc->user_ns)
> > return -EIO;
> >
> > restart:
> > @@ -1880,7 +1882,8 @@ static ssize_t fuse_dev_do_write(struct fuse_dev *fud,
> > struct fuse_req *req;
> > struct fuse_out_header oh;
> >
> > - if (task_active_pid_ns(current) != fc->pid_ns)
> > + if (task_active_pid_ns(current) != fc->pid_ns ||
> > + current_user_ns() != fc->user_ns)
> > return -EIO;
> >
> > if (nbytes < sizeof(struct fuse_out_header))
> > diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> > index 5e2e08712d3b..8fd9fe4dcd43 100644
> > --- a/fs/fuse/dir.c
> > +++ b/fs/fuse/dir.c
> > @@ -841,8 +841,8 @@ static void fuse_fillattr(struct inode *inode, struct fuse_attr *attr,
> > stat->ino = attr->ino;
> > stat->mode = (inode->i_mode & S_IFMT) | (attr->mode & 07777);
> > stat->nlink = attr->nlink;
> > - stat->uid = make_kuid(&init_user_ns, attr->uid);
> > - stat->gid = make_kgid(&init_user_ns, attr->gid);
> > + stat->uid = inode->i_uid;
> > + stat->gid = inode->i_gid;
>
> This breaks the attr_version logic in fuse_change_attributes().
>
> So just use make_k[ug]id() here as well.
Okay.
> > stat->rdev = inode->i_rdev;
> > stat->atime.tv_sec = attr->atime;
> > stat->atime.tv_nsec = attr->atimensec;
> > @@ -1455,17 +1455,17 @@ static bool update_mtime(unsigned ivalid, bool trust_local_mtime)
> > return true;
> > }
> >
> > -static void iattr_to_fattr(struct iattr *iattr, struct fuse_setattr_in *arg,
> > - bool trust_local_cmtime)
> > +static void iattr_to_fattr(struct fuse_conn *fc, struct iattr *iattr,
> > + struct fuse_setattr_in *arg, bool trust_local_cmtime)
> > {
> > unsigned ivalid = iattr->ia_valid;
> >
> > if (ivalid & ATTR_MODE)
> > arg->valid |= FATTR_MODE, arg->mode = iattr->ia_mode;
> > if (ivalid & ATTR_UID)
> > - arg->valid |= FATTR_UID, arg->uid = from_kuid(&init_user_ns, iattr->ia_uid);
> > + arg->valid |= FATTR_UID, arg->uid = from_kuid(fc->user_ns, iattr->ia_uid);
> > if (ivalid & ATTR_GID)
> > - arg->valid |= FATTR_GID, arg->gid = from_kgid(&init_user_ns, iattr->ia_gid);
> > + arg->valid |= FATTR_GID, arg->gid = from_kgid(fc->user_ns, iattr->ia_gid);
> > if (ivalid & ATTR_SIZE)
> > arg->valid |= FATTR_SIZE, arg->size = iattr->ia_size;
> > if (ivalid & ATTR_ATIME) {
> > @@ -1625,7 +1625,7 @@ int fuse_do_setattr(struct inode *inode, struct iattr *attr,
> >
> > memset(&inarg, 0, sizeof(inarg));
> > memset(&outarg, 0, sizeof(outarg));
> > - iattr_to_fattr(attr, &inarg, trust_local_cmtime);
> > + iattr_to_fattr(fc, attr, &inarg, trust_local_cmtime);
> > if (file) {
> > struct fuse_file *ff = file->private_data;
> > inarg.valid |= FATTR_FH;
> > diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> > index 143b595197b6..5897805405ba 100644
> > --- a/fs/fuse/fuse_i.h
> > +++ b/fs/fuse/fuse_i.h
> > @@ -23,6 +23,7 @@
> > #include <linux/poll.h>
> > #include <linux/workqueue.h>
> > #include <linux/pid_namespace.h>
> > +#include <linux/user_namespace.h>
> >
> > /** Max number of pages that can be used in a single read request */
> > #define FUSE_MAX_PAGES_PER_REQ 32
> > @@ -460,6 +461,9 @@ struct fuse_conn {
> > /** The pid namespace for this mount */
> > struct pid_namespace *pid_ns;
> >
> > + /** The user namespace for this mount */
> > + struct user_namespace *user_ns;
> > +
> > /** The fuse mount flags for this mount */
> > unsigned flags;
> >
> > @@ -855,7 +859,7 @@ struct fuse_conn *fuse_conn_get(struct fuse_conn *fc);
> > /**
> > * Initialize fuse_conn
> > */
> > -void fuse_conn_init(struct fuse_conn *fc);
> > +void fuse_conn_init(struct fuse_conn *fc, struct user_namespace *user_ns);
> >
> > /**
> > * Release reference to fuse_conn
> > diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> > index 2f31874ea9db..b7bdfdac3521 100644
> > --- a/fs/fuse/inode.c
> > +++ b/fs/fuse/inode.c
> > @@ -167,8 +167,8 @@ void fuse_change_attributes_common(struct inode *inode, struct fuse_attr *attr,
> > inode->i_ino = fuse_squash_ino(attr->ino);
> > inode->i_mode = (inode->i_mode & S_IFMT) | (attr->mode & 07777);
> > set_nlink(inode, attr->nlink);
> > - inode->i_uid = make_kuid(&init_user_ns, attr->uid);
> > - inode->i_gid = make_kgid(&init_user_ns, attr->gid);
> > + inode->i_uid = make_kuid(fc->user_ns, attr->uid);
> > + inode->i_gid = make_kgid(fc->user_ns, attr->gid);
> > inode->i_blocks = attr->blocks;
> > inode->i_atime.tv_sec = attr->atime;
> > inode->i_atime.tv_nsec = attr->atimensec;
> > @@ -467,12 +467,15 @@ static int fuse_match_uint(substring_t *s, unsigned int *res)
> > return err;
> > }
> >
> > -static int parse_fuse_opt(char *opt, struct fuse_mount_data *d, int is_bdev)
> > +static int parse_fuse_opt(char *opt, struct fuse_mount_data *d, int is_bdev,
> > + struct user_namespace *user_ns)
> > {
> > char *p;
> > memset(d, 0, sizeof(struct fuse_mount_data));
> > d->max_read = ~0;
> > d->blksize = FUSE_DEFAULT_BLKSIZE;
> > + d->user_id = make_kuid(user_ns, 0);
> > + d->group_id = make_kgid(user_ns, 0);
>
> It is true that if "user_id=" or "group_id" options were omitted we used the
> zero uid/gid values. However, this isn't actually used by anybody AFAIK, and
> generalizing it for userns doesn't seem to make much sense.
>
> So I suggest we that we instead return an error if mounting from a userns AND
> neither "allow_other" nor both "user_id" and "group_id" are specified.
But those are also used for ownership of the connection files in
fusectl. In an allow_other mount shouldn't those files by owned by
namespace root and not global root?
> > while ((p = strsep(&opt, ",")) != NULL) {
> > int token;
> > @@ -503,7 +506,7 @@ static int parse_fuse_opt(char *opt, struct fuse_mount_data *d, int is_bdev)
> > case OPT_USER_ID:
> > if (fuse_match_uint(&args[0], &uv))
> > return 0;
> > - d->user_id = make_kuid(current_user_ns(), uv);
> > + d->user_id = make_kuid(user_ns, uv);
> > if (!uid_valid(d->user_id))
> > return 0;
> > d->user_id_present = 1;
> > @@ -512,7 +515,7 @@ static int parse_fuse_opt(char *opt, struct fuse_mount_data *d, int is_bdev)
> > case OPT_GROUP_ID:
> > if (fuse_match_uint(&args[0], &uv))
> > return 0;
> > - d->group_id = make_kgid(current_user_ns(), uv);
> > + d->group_id = make_kgid(user_ns, uv);
> > if (!gid_valid(d->group_id))
> > return 0;
> > d->group_id_present = 1;
> > @@ -555,8 +558,10 @@ static int fuse_show_options(struct seq_file *m, struct dentry *root)
> > struct super_block *sb = root->d_sb;
> > struct fuse_conn *fc = get_fuse_conn_super(sb);
> >
> > - seq_printf(m, ",user_id=%u", from_kuid_munged(&init_user_ns, fc->user_id));
> > - seq_printf(m, ",group_id=%u", from_kgid_munged(&init_user_ns, fc->group_id));
> > + seq_printf(m, ",user_id=%u",
> > + from_kuid_munged(fc->user_ns, fc->user_id));
> > + seq_printf(m, ",group_id=%u",
> > + from_kgid_munged(fc->user_ns, fc->group_id));
> > if (fc->flags & FUSE_DEFAULT_PERMISSIONS)
> > seq_puts(m, ",default_permissions");
> > if (fc->flags & FUSE_ALLOW_OTHER)
> > @@ -587,7 +592,7 @@ static void fuse_pqueue_init(struct fuse_pqueue *fpq)
> > fpq->connected = 1;
> > }
> >
> > -void fuse_conn_init(struct fuse_conn *fc)
> > +void fuse_conn_init(struct fuse_conn *fc, struct user_namespace *user_ns)
> > {
> > memset(fc, 0, sizeof(*fc));
> > spin_lock_init(&fc->lock);
> > @@ -611,6 +616,7 @@ void fuse_conn_init(struct fuse_conn *fc)
> > fc->attr_version = 1;
> > get_random_bytes(&fc->scramble_key, sizeof(fc->scramble_key));
> > fc->pid_ns = get_pid_ns(task_active_pid_ns(current));
> > + fc->user_ns = get_user_ns(user_ns);
> > }
> > EXPORT_SYMBOL_GPL(fuse_conn_init);
> >
> > @@ -620,6 +626,7 @@ void fuse_conn_put(struct fuse_conn *fc)
> > if (fc->destroy_req)
> > fuse_request_free(fc->destroy_req);
> > put_pid_ns(fc->pid_ns);
> > + put_user_ns(fc->user_ns);
> > fc->release(fc);
> > }
> > }
> > @@ -1046,7 +1053,7 @@ static int fuse_fill_super(struct super_block *sb, void *data, int silent)
> >
> > sb->s_flags &= ~(MS_NOSEC | MS_I_VERSION);
> >
> > - if (!parse_fuse_opt(data, &d, is_bdev))
> > + if (!parse_fuse_opt(data, &d, is_bdev, sb->s_user_ns))
> > goto err;
> >
> > if (is_bdev) {
> > @@ -1070,8 +1077,12 @@ static int fuse_fill_super(struct super_block *sb, void *data, int silent)
> > if (!file)
> > goto err;
> >
> > - if ((file->f_op != &fuse_dev_operations) ||
> > - (file->f_cred->user_ns != &init_user_ns))
> > + /*
> > + * Require mount to happen from the same user namespace which
> > + * opened /dev/fuse to prevent potential attacks.
> > + */
> > + if (file->f_op != &fuse_dev_operations ||
> > + file->f_cred->user_ns != sb->s_user_ns)
> > goto err_fput;
> >
> > fc = kmalloc(sizeof(*fc), GFP_KERNEL);
> > @@ -1079,7 +1090,7 @@ static int fuse_fill_super(struct super_block *sb, void *data, int silent)
> > if (!fc)
> > goto err_fput;
> >
> > - fuse_conn_init(fc);
> > + fuse_conn_init(fc, sb->s_user_ns);
> > fc->release = fuse_free_conn;
> >
> > fud = fuse_dev_alloc(fc);
> > --
> > 1.9.1
> >
^ permalink raw reply
* Re: [PATCH RESEND v2 15/18] fuse: Add support for pid namespaces
From: Seth Forshee @ 2016-03-09 14:17 UTC (permalink / raw)
To: Miklos Szeredi
Cc: Eric W. Biederman, Alexander Viro, Serge Hallyn,
Richard Weinberger, Austin S Hemmelgarn, linux-kernel,
linux-bcache, dm-devel, linux-raid, linux-mtd, linux-fsdevel,
fuse-devel, linux-security-module, selinux, Miklos Szeredi
In-Reply-To: <20160309105317.GE8655@tucsk>
On Wed, Mar 09, 2016 at 11:53:17AM +0100, Miklos Szeredi wrote:
> On Mon, Jan 04, 2016 at 12:03:54PM -0600, Seth Forshee wrote:
> > If the userspace process servicing fuse requests is running in
> > a pid namespace then pids passed via the fuse fd need to be
> > translated relative to that namespace. Capture the pid namespace
> > in use when the filesystem is mounted and use this for pid
> > translation.
> >
> > Since no use case currently exists for changing namespaces all
> > translations are done relative to the pid namespace in use when
> > /dev/fuse is opened.
>
> The above doesn't match what the patch does.
>
> - FUSE captures namespace at mount time
>
> - CUSE captures namespace at /dev/cuse open
Possibly an earlier version of the patch worked that way and I forgot to
update the description after it change. Anyway, I'll fix it.
> > Mounting or /dev/fuse IO from another
> > namespace will return errors.
> >
> > Requests from processes whose pid cannot be translated into the
> > target namespace are not permitted, except for requests
> > allocated via fuse_get_req_nofail_nopages. For no-fail requests
> > in.h.pid will be 0 if the pid translation fails.
> >
> > File locking changes based on previous work done by Eric
> > Biederman.
> >
> > Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
> > Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
>
> Not sure how my SOB got on this patch, use this instead:
>
> Acked-by: Miklos Szeredi <mszeredi@redhat.com>
My memory is that you had sent a patch as a proposed alternative to one
of my earlier patches, and I squashed the two together and added your
SOB at that point. I'll change it.
Thanks,
Seth
^ permalink raw reply
* Re: [PATCH 4/8] Grow: Grow_addbitmap(): Add check to quiet down static code checkers
From: Jes Sorensen @ 2016-03-09 14:00 UTC (permalink / raw)
To: Guoqing Jiang; +Cc: linux-raid, neilb, pawel.baldysiak
In-Reply-To: <56E0606E.5000906@suse.com>
Guoqing Jiang <gqjiang@suse.com> writes:
> On 03/09/2016 01:30 AM, Jes.Sorensen@redhat.com wrote:
>> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>>
>> Grow_addbitmap() is only ever called with s->bitmap_file != NULL, but
>> not all static code checkers catch this. This adds a check to quiet
>> down the false positive warnings.
>>
>> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
>> ---
>> Grow.c | 9 ++++++++-
>> 1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/Grow.c b/Grow.c
>> index 0fa776d..c453eb6 100755
>> --- a/Grow.c
>> +++ b/Grow.c
>> @@ -297,7 +297,14 @@ int Grow_addbitmap(char *devname, int fd, struct context *c, struct shape *s)
>> " between different architectures. Consider upgrading the Linux kernel.\n");
>> }
>> - if (s->bitmap_file && strcmp(s->bitmap_file, "clustered") ==
>> 0)
>> + /*
>> + * We only ever get called if s->bitmap_file is != NULL, so this check
>> + * is just here to quiet down static code checkers.
>> + */
>> + if (!s->bitmap_file)
>> + return 1;
>
> Is it really need to make all static code checkers happy? ;-)
> Otherwise, I would prefer remove above check.
>
> Anyway, I am fine with the changes.
We had a check in one place, but not in the remaining places. I just
made it more consistent.
Making the code checker happy does make some sense because it finds
valid bugs too, and they are easier to find when we don't get a lot of
false warnings.
Thanks for your review :)
Cheers,
Jes
^ permalink raw reply
* Re: [PATCH RESEND v2 18/18] fuse: Allow user namespace mounts
From: Miklos Szeredi @ 2016-03-09 13:08 UTC (permalink / raw)
To: Seth Forshee
Cc: Eric W. Biederman, Alexander Viro, Serge Hallyn,
Richard Weinberger, Austin S Hemmelgarn, linux-kernel,
linux-bcache, dm-devel, linux-raid, linux-mtd, linux-fsdevel,
fuse-devel, linux-security-module, selinux
In-Reply-To: <1451930639-94331-19-git-send-email-seth.forshee@canonical.com>
On Mon, Jan 04, 2016 at 12:03:57PM -0600, Seth Forshee wrote:
> Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
Acked-by: Miklos Szeredi <mszeredi@redhat.com>
> ---
> fs/fuse/inode.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> index b7bdfdac3521..2fd338c199ce 100644
> --- a/fs/fuse/inode.c
> +++ b/fs/fuse/inode.c
> @@ -1201,7 +1201,7 @@ static void fuse_kill_sb_anon(struct super_block *sb)
> static struct file_system_type fuse_fs_type = {
> .owner = THIS_MODULE,
> .name = "fuse",
> - .fs_flags = FS_HAS_SUBTYPE,
> + .fs_flags = FS_HAS_SUBTYPE | FS_USERNS_MOUNT,
> .mount = fuse_mount,
> .kill_sb = fuse_kill_sb_anon,
> };
> @@ -1233,7 +1233,7 @@ static struct file_system_type fuseblk_fs_type = {
> .name = "fuseblk",
> .mount = fuse_mount_blk,
> .kill_sb = fuse_kill_sb_blk,
> - .fs_flags = FS_REQUIRES_DEV | FS_HAS_SUBTYPE,
> + .fs_flags = FS_REQUIRES_DEV | FS_HAS_SUBTYPE | FS_USERNS_MOUNT,
> };
> MODULE_ALIAS_FS("fuseblk");
>
> --
> 1.9.1
>
^ permalink raw reply
* Re: [PATCH RESEND v2 17/18] fuse: Restrict allow_other to the superblock's namespace or a descendant
From: Miklos Szeredi @ 2016-03-09 11:40 UTC (permalink / raw)
To: Seth Forshee
Cc: Eric W. Biederman, Alexander Viro, Serge Hallyn,
Richard Weinberger, Austin S Hemmelgarn, linux-kernel,
linux-bcache, dm-devel, linux-raid, linux-mtd, linux-fsdevel,
fuse-devel, linux-security-module, selinux
In-Reply-To: <1451930639-94331-18-git-send-email-seth.forshee@canonical.com>
On Mon, Jan 04, 2016 at 12:03:56PM -0600, Seth Forshee wrote:
> Unprivileged users are normally restricted from mounting with the
> allow_other option by system policy, but this could be bypassed
> for a mount done with user namespace root permissions. In such
> cases allow_other should not allow users outside the userns
> to access the mount as doing so would give the unprivileged user
> the ability to manipulate processes it would otherwise be unable
> to manipulate. Restrict allow_other to apply to users in the same
> userns used at mount or a descendant of that namespace.
>
> Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
> Acked-by: Serge Hallyn <serge.hallyn@canonical.com>
Acked-by: Miklos Szeredi <mszeredi@redhat.com>
> ---
> fs/fuse/dir.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> index 8fd9fe4dcd43..24e4cdb554f1 100644
> --- a/fs/fuse/dir.c
> +++ b/fs/fuse/dir.c
> @@ -1015,7 +1015,7 @@ int fuse_allow_current_process(struct fuse_conn *fc)
> const struct cred *cred;
>
> if (fc->flags & FUSE_ALLOW_OTHER)
> - return 1;
> + return current_in_userns(fc->user_ns);
>
> cred = current_cred();
> if (uid_eq(cred->euid, fc->user_id) &&
> --
> 1.9.1
>
^ 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