NetBSD Problem Report #53217
From kardel@mail.kardel.name Thu Apr 26 19:06:33 2018
Return-Path: <kardel@mail.kardel.name>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
(Client CN "mail.NetBSD.org", Issuer "mail.NetBSD.org CA" (not verified))
by mollari.NetBSD.org (Postfix) with ESMTPS id 5D3F77A1E5
for <gnats-bugs@gnats.NetBSD.org>; Thu, 26 Apr 2018 19:06:33 +0000 (UTC)
Message-Id: <20180426190628.B1A0628DB83@mail.kardel.name>
Date: Thu, 26 Apr 2018 19:06:28 +0000 (UTC)
From: kardel@netbsd.org
Reply-To: kardel@netbsd.org
To: gnats-bugs@NetBSD.org
Subject: -current: dumping to wedges on GPT disks broken
X-Send-Pr-Version: 3.95
>Number: 53217
>Category: kern
>Synopsis: -current: dumping to wedges on GPT disks broken
>Confidential: no
>Severity: serious
>Priority: medium
>Responsible: mlelstv
>State: closed
>Class: sw-bug
>Submitter-Id: net
>Arrival-Date: Thu Apr 26 19:10:00 +0000 2018
>Closed-Date: Mon Jun 03 06:46:07 +0000 2019
>Last-Modified: Mon Jun 03 06:46:07 +0000 2019
>Originator: Frank Kardel
>Release: NetBSD 8.99.14
>Organization:
>Environment:
System: NetBSD gateway 8.99.14 NetBSD 8.99.14 (GATEWAY) #8: Tue Apr 24 19:32:42 CEST 2018 kardel@xxx:/src/NetBSD/act/src/obj.amd64/sys/arch/amd64/compile/GATEWAY amd64
Architecture: x86_64
Machine: amd64
>Description:
dumping on a swap wedge from a GPT partitioned disk fails with
dump: device bad
caused by: dksubr.c:dk_dump():806
806 p = &lp->d_partitions[part];
807 if (p->p_fstype != FS_SWAP) {
808 DPRINTF(DKDB_DUMP, ("%s: bad fstype %d\n", __func__,
809 p->p_fstype));
810 return ENXIO;
811 }
fstype is 0 here (FS_UNUSED) as a wedge on a GPT labeled disk references
the whole disk as underlying structure which usually has FS_UNUSED as type.
>How-To-Repeat:
create a GPT partitions disk with a swap partition. set dump device to
the swap wedge. attempt to dump.
>Fix:
Maybe also accept FS_UNUSED as fstype?
>Release-Note:
>Audit-Trail:
From: Martin Husemann <martin@duskware.de>
To: gnats-bugs@NetBSD.org
Cc:
Subject: Re: kern/53217: -current: dumping to wedges on GPT disks broken
Date: Fri, 27 Apr 2018 09:42:05 +0200
On Thu, Apr 26, 2018 at 07:10:00PM +0000, kardel@netbsd.org wrote:
> >Fix:
> Maybe also accept FS_UNUSED as fstype?
I'd rather have the dkwedge_gpt.c code set the partition type of the
fake label for the whole disk of the wedge to FS_SWAP when dkw.dkw_ptype
is DKW_PTYPE_SWAP.
Martin
From: Frank Kardel <kardel@netbsd.org>
To: gnats-bugs@NetBSD.org
Cc:
Subject: Re: kern/53217: -current: dumping to wedges on GPT disks broken
Date: Fri, 27 Apr 2018 15:01:25 +0200
As long as it works, fine.
It violates, though, a bit the principle of least astonishment.
sometimes GPT disks would have a full disk partition with "swap"
sometime with "unused" - both are not really describing the GPT label
situation. But this should not be a long discussion - getting dumps
again on EFI systems is more important.
Frank
On 04/27/18 09:45, Martin Husemann wrote:
> The following reply was made to PR kern/53217; it has been noted by GNATS.
>
> From: Martin Husemann <martin@duskware.de>
> To: gnats-bugs@NetBSD.org
> Cc:
> Subject: Re: kern/53217: -current: dumping to wedges on GPT disks broken
> Date: Fri, 27 Apr 2018 09:42:05 +0200
>
> On Thu, Apr 26, 2018 at 07:10:00PM +0000, kardel@netbsd.org wrote:
> > >Fix:
> > Maybe also accept FS_UNUSED as fstype?
>
> I'd rather have the dkwedge_gpt.c code set the partition type of the
> fake label for the whole disk of the wedge to FS_SWAP when dkw.dkw_ptype
> is DKW_PTYPE_SWAP.
>
> Martin
>
From: Martin Husemann <martin@duskware.de>
To: gnats-bugs@NetBSD.org
Cc:
Subject: Re: kern/53217: -current: dumping to wedges on GPT disks broken
Date: Fri, 27 Apr 2018 15:15:38 +0200
On Fri, Apr 27, 2018 at 01:05:01PM +0000, Frank Kardel wrote:
> As long as it works, fine.
Everything else is pretty dangerous ;-)
> It violates, though, a bit the principle of least astonishment.
> sometimes GPT disks would have a full disk partition with "swap"
> sometime with "unused"
I think there is confusion which disk devices "label" we are talking about
here (and what the code checks).
The full disk "partition" of the fictious (purely internal) label on my
dk1 partition better be of type swap, while my dk0 better be ffs:
wd0: GPT GUID: d96ce1d6-f47b-464b-b0c8-b8fea0d23f31
dk0 at wd0: Guru-Root
dk0: 5856338797 blocks at 34, type: ffs
dk1 at wd0: d5c3eff7-a35d-41b3-9c58-76679533f5cb
dk1: 4194304 blocks at 5856338831, type: swap
wd0: drive supports PIO mode 4, DMA mode 2, Ultra-DMA mode 6 (Ultra/133)
But wd0's internal label should have a raw partition of type unused.
The latter is true, the former two I can't check easily as dk* does not
provide access to the label (which is good too).
Martin
From: Frank Kardel <kardel@netbsd.org>
To: gnats-bugs@NetBSD.org
Cc:
Subject: Re: kern/53217: -current: dumping to wedges on GPT disks broken
Date: Fri, 27 Apr 2018 15:48:20 +0200
You are right in that regard. The problem is not the dkX types - they
are fine an correct, but when the dump function crawls into the rabbit
hole accessing the underlying disk it uses the whole disk partion of the
underlying disk with the correct offset applied:
dk.c:dkdump()
1624 bdev = bdevsw_lookup(sc->sc_pdev);
1625 rv = (*bdev->d_dump)(sc->sc_pdev, blkno +
sc->sc_offset, va, size);
Then the dump function of the wd driver is called and defers the work to
wd.c:wddump()
1420 dksc = &wd->sc_dksc;
1421
1422 return dk_dump(dksc, dev, blkno, va, size);
In dksubr.c dkdump (not the wedge code, the general disk support) then
examines as I quoted
796 /* Convert to disk sectors. Request must be a
multiple of size. */
797 part = DISKPART(dev);
...
806 p = &lp->d_partitions[part];
807 if (p->p_fstype != FS_SWAP && p->p_fstype != FS_UNUSED) {
808 DPRINTF(DKDB_DUMP, ("%s: bad fstype %d\n",
__func__,
809 p->p_fstype));
810 return ENXIO;
811 }
and finds FS_UNUSED as it is now looking at the full disk "partition" it
got from sc->sc_pdev in dkwedge/dk.c. Then
ENXIO is return and that is the end of the dump.
I am not talking about the dk labels here, but of the label of the
underlying disk.
My setup is:
/dev/rwd0: 4 wedges:
dk0: EFI System, 262144 blocks at 2048, type: msdos
dk1: bootswap, 33554432 blocks at 264192, type: swap
dk2: System Root, 419430400 blocks at 33818624, type: ffs
dk3: User, 1500276111 blocks at 453249024, type: ffs
disklabel wd0 (internal label - automagically created)
...
4 partitions:
# size offset fstype [fsize bsize cpg/sgs]
a: 1953525168 0 4.2BSD 0 0 0 # (Cyl. 0 -
1938020)
d: 1953525168 0 unused 0 0 # (Cyl. 0 -
1938020)
gpt show wd0
start size index contents
0 1 PMBR
1 1 Pri GPT header
2 32 Pri GPT table
34 2014 Unused
2048 262144 1 GPT part - EFI System
264192 33554432 2 GPT part - NetBSD swap
33818624 419430400 3 GPT part - NetBSD FFSv1/FFSv2
453249024 1500276111 4 GPT part - NetBSD FFSv1/FFSv2
1953525135 32 Sec GPT table
1953525167 1 Sec GPT header
Everything is fine until the dksubr.c:dkdump routing hits partition d on
the underlying disk.
Frank
On 04/27/18 15:20, Martin Husemann wrote:
> The following reply was made to PR kern/53217; it has been noted by GNATS.
>
> From: Martin Husemann <martin@duskware.de>
> To: gnats-bugs@NetBSD.org
> Cc:
> Subject: Re: kern/53217: -current: dumping to wedges on GPT disks broken
> Date: Fri, 27 Apr 2018 15:15:38 +0200
>
> On Fri, Apr 27, 2018 at 01:05:01PM +0000, Frank Kardel wrote:
> > As long as it works, fine.
>
> Everything else is pretty dangerous ;-)
>
> > It violates, though, a bit the principle of least astonishment.
> > sometimes GPT disks would have a full disk partition with "swap"
> > sometime with "unused"
>
> I think there is confusion which disk devices "label" we are talking about
> here (and what the code checks).
>
> The full disk "partition" of the fictious (purely internal) label on my
> dk1 partition better be of type swap, while my dk0 better be ffs:
>
> wd0: GPT GUID: d96ce1d6-f47b-464b-b0c8-b8fea0d23f31
> dk0 at wd0: Guru-Root
> dk0: 5856338797 blocks at 34, type: ffs
> dk1 at wd0: d5c3eff7-a35d-41b3-9c58-76679533f5cb
> dk1: 4194304 blocks at 5856338831, type: swap
> wd0: drive supports PIO mode 4, DMA mode 2, Ultra-DMA mode 6 (Ultra/133)
>
> But wd0's internal label should have a raw partition of type unused.
>
> The latter is true, the former two I can't check easily as dk* does not
> provide access to the label (which is good too).
>
>
> Martin
>
From: Martin Husemann <martin@duskware.de>
To: gnats-bugs@NetBSD.org
Cc:
Subject: Re: kern/53217: -current: dumping to wedges on GPT disks broken
Date: Fri, 27 Apr 2018 15:53:58 +0200
On Fri, Apr 27, 2018 at 01:50:01PM +0000, Frank Kardel wrote:
> dk.c:dkdump()
> 1624 bdev = bdevsw_lookup(sc->sc_pdev);
> 1625 rv = (*bdev->d_dump)(sc->sc_pdev, blkno +
> sc->sc_offset, va, size);
OK, I missed the parent device here.
> In dksubr.c dkdump (not the wedge code, the general disk support) then
> examines as I quoted
> 796 /* Convert to disk sectors. Request must be a
> multiple of size. */
> 797 part = DISKPART(dev);
> ...
> 806 p = &lp->d_partitions[part];
And here accessing the label at all is clearly a bug.
We need to remove that check and move it into the callers.
Martin
From: Frank Kardel <kardel@netbsd.org>
To: gnats-bugs@NetBSD.org
Cc:
Subject: Re: kern/53217: -current: dumping to wedges on GPT disks broken
Date: Fri, 27 Apr 2018 16:42:53 +0200
That would be the dump functions in the disk drivers, but that have the
same problem as parent devices, they would still be looking at "whole
disk" in the GPT case. In the GPT case only dkwedge/dk.c knows about the
swap type. In the BSD label case the check would make sense.
The check would only be valid for non whole disk partitions in the disk
drivers or even in this place or am I missing something?
Frank
On 04/27/18 15:55, Martin Husemann wrote:
> The following reply was made to PR kern/53217; it has been noted by GNATS.
>
> From: Martin Husemann <martin@duskware.de>
> To: gnats-bugs@NetBSD.org
> Cc:
> Subject: Re: kern/53217: -current: dumping to wedges on GPT disks broken
> Date: Fri, 27 Apr 2018 15:53:58 +0200
>
> On Fri, Apr 27, 2018 at 01:50:01PM +0000, Frank Kardel wrote:
>
> > dk.c:dkdump()
> > 1624 bdev = bdevsw_lookup(sc->sc_pdev);
> > 1625 rv = (*bdev->d_dump)(sc->sc_pdev, blkno +
> > sc->sc_offset, va, size);
>
> OK, I missed the parent device here.
>
> > In dksubr.c dkdump (not the wedge code, the general disk support) then
> > examines as I quoted
> > 796 /* Convert to disk sectors. Request must be a
> > multiple of size. */
> > 797 part = DISKPART(dev);
> > ...
> > 806 p = &lp->d_partitions[part];
>
> And here accessing the label at all is clearly a bug.
>
> We need to remove that check and move it into the callers.
>
> Martin
>
From: mlelstv@serpens.de (Michael van Elst)
To: gnats-bugs@netbsd.org
Cc:
Subject: Re: kern/53217: -current: dumping to wedges on GPT disks broken
Date: Fri, 27 Apr 2018 15:15:03 -0000 (UTC)
kardel@netbsd.org writes:
>Machine: amd64
>>Description:
> dumping on a swap wedge from a GPT partitioned disk fails with
> dump: device bad
> caused by: dksubr.c:dk_dump():806
> 806 p = &lp->d_partitions[part];
> 807 if (p->p_fstype != FS_SWAP) {
> 808 DPRINTF(DKDB_DUMP, ("%s: bad fstype %d\n", __func__,
> 809 p->p_fstype));
> 810 return ENXIO;
> 811 }
The wedge references the RAW_PART of the parent disk, which rarely has
a valid fstype.
So adding the case RAW_PART && FS_UNUSED as allowed should fix the
issue.
--
--
Michael van Elst
Internet: mlelstv@serpens.de
"A potential Snark may lurk in every tree."
From: christos@zoulas.com (Christos Zoulas)
To: gnats-bugs@NetBSD.org, kern-bug-people@netbsd.org,
gnats-admin@netbsd.org, netbsd-bugs@netbsd.org, kardel@netbsd.org
Cc:
Subject: Re: kern/53217: -current: dumping to wedges on GPT disks broken
Date: Fri, 27 Apr 2018 11:35:25 -0400
On Apr 27, 2:45pm, kardel@netbsd.org (Frank Kardel) wrote:
-- Subject: Re: kern/53217: -current: dumping to wedges on GPT disks broken
| That would be the dump functions in the disk drivers, but that have the
| same problem as parent devices, they would still be looking at "whole
| disk" in the GPT case. In the GPT case only dkwedge/dk.c knows about the
| swap type. In the BSD label case the check would make sense.
| The check would only be valid for non whole disk partitions in the disk
| drivers or even in this place or am I missing something?
I think that we should either fix the wedge code to create a better
fictitious disklabel or get rid of the label code and have it just
be there for compatibility (replacing dk_label with something that
works in all cases).
christos
From: Jason Thorpe <thorpej@me.com>
To: Christos Zoulas <christos@zoulas.com>
Cc: gnats-bugs@NetBSD.org, kern-bug-people@netbsd.org, netbsd-bugs@netbsd.org,
kardel@netbsd.org
Subject: Re: kern/53217: -current: dumping to wedges on GPT disks broken
Date: Fri, 27 Apr 2018 08:40:32 -0700
--Apple-Mail=_E03DE96F-24E2-4AA7-8AB7-0027A868B57E
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain;
charset=utf-8
> On Apr 27, 2018, at 8:35 AM, Christos Zoulas <christos@zoulas.com> =
wrote:
>=20
> I think that we should either fix the wedge code to create a better
> fictitious disklabel or get rid of the label code and have it just
> be there for compatibility (replacing dk_label with something that
> works in all cases).
The original intent of the wedge code is that it would subsume all of =
the partition format schemes, and that =E2=80=9Cstruct disklabel=E2=80=9D =
would remain for on-disk format compatibility only.
In a perfect world, all platforms that don=E2=80=99t otherwise have a =
=E2=80=9Cnative=E2=80=9D partition scheme (i.e. something that=E2=80=99s =
understood directly by, say, the firmware) would migrate to GPT, since =
it=E2=80=99s cross-platform and extremely flexible.
-- thorpej
--Apple-Mail=_E03DE96F-24E2-4AA7-8AB7-0027A868B57E
Content-Transfer-Encoding: quoted-printable
Content-Type: text/html;
charset=utf-8
<html><head><meta http-equiv=3D"Content-Type" content=3D"text/html; =
charset=3Dutf-8"></head><body style=3D"word-wrap: break-word; =
-webkit-nbsp-mode: space; line-break: after-white-space;" class=3D""><br =
class=3D""><div><br class=3D""><blockquote type=3D"cite" class=3D""><div =
class=3D"">On Apr 27, 2018, at 8:35 AM, Christos Zoulas <<a =
href=3D"mailto:christos@zoulas.com" class=3D"">christos@zoulas.com</a>>=
wrote:</div><br class=3D"Apple-interchange-newline"><div class=3D""><span=
style=3D"caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: =
12px; font-style: normal; font-variant-caps: normal; font-weight: =
normal; letter-spacing: normal; text-align: start; text-indent: 0px; =
text-transform: none; white-space: normal; word-spacing: 0px; =
-webkit-text-stroke-width: 0px; text-decoration: none; float: none; =
display: inline !important;" class=3D"">I think that we should either =
fix the wedge code to create a better</span><br style=3D"caret-color: =
rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: =
normal; font-variant-caps: normal; font-weight: normal; letter-spacing: =
normal; text-align: start; text-indent: 0px; text-transform: none; =
white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; =
text-decoration: none;" class=3D""><span style=3D"caret-color: rgb(0, 0, =
0); font-family: Helvetica; font-size: 12px; font-style: normal; =
font-variant-caps: normal; font-weight: normal; letter-spacing: normal; =
text-align: start; text-indent: 0px; text-transform: none; white-space: =
normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; =
text-decoration: none; float: none; display: inline !important;" =
class=3D"">fictitious disklabel or get rid of the label code and have it =
just</span><br style=3D"caret-color: rgb(0, 0, 0); font-family: =
Helvetica; font-size: 12px; font-style: normal; font-variant-caps: =
normal; font-weight: normal; letter-spacing: normal; text-align: start; =
text-indent: 0px; text-transform: none; white-space: normal; =
word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: =
none;" class=3D""><span style=3D"caret-color: rgb(0, 0, 0); font-family: =
Helvetica; font-size: 12px; font-style: normal; font-variant-caps: =
normal; font-weight: normal; letter-spacing: normal; text-align: start; =
text-indent: 0px; text-transform: none; white-space: normal; =
word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: =
none; float: none; display: inline !important;" class=3D"">be there for =
compatibility (replacing dk_label with something that</span><br =
style=3D"caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: =
12px; font-style: normal; font-variant-caps: normal; font-weight: =
normal; letter-spacing: normal; text-align: start; text-indent: 0px; =
text-transform: none; white-space: normal; word-spacing: 0px; =
-webkit-text-stroke-width: 0px; text-decoration: none;" class=3D""><span =
style=3D"caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: =
12px; font-style: normal; font-variant-caps: normal; font-weight: =
normal; letter-spacing: normal; text-align: start; text-indent: 0px; =
text-transform: none; white-space: normal; word-spacing: 0px; =
-webkit-text-stroke-width: 0px; text-decoration: none; float: none; =
display: inline !important;" class=3D"">works in all cases).</span><br =
style=3D"caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: =
12px; font-style: normal; font-variant-caps: normal; font-weight: =
normal; letter-spacing: normal; text-align: start; text-indent: 0px; =
text-transform: none; white-space: normal; word-spacing: 0px; =
-webkit-text-stroke-width: 0px; text-decoration: none;" =
class=3D""></div></blockquote></div><div class=3D""><br =
class=3D""></div><div class=3D"">The original intent of the wedge code =
is that it would subsume all of the partition format schemes, and that =
=E2=80=9Cstruct disklabel=E2=80=9D would remain for on-disk format =
compatibility only.</div><div class=3D""><br class=3D""></div><div =
class=3D"">In a perfect world, all platforms that don=E2=80=99t =
otherwise have a =E2=80=9Cnative=E2=80=9D partition scheme (i.e. =
something that=E2=80=99s understood directly by, say, the firmware) =
would migrate to GPT, since it=E2=80=99s cross-platform and extremely =
flexible.</div><br class=3D""><div class=3D"">
<span class=3D"Apple-style-span" style=3D"border-collapse: separate; =
color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; =
font-style: normal; font-variant: normal; font-weight: normal; =
letter-spacing: normal; line-height: normal; orphans: 2; text-align: =
auto; text-indent: 0px; text-transform: none; white-space: normal; =
widows: 2; word-spacing: 0px; -webkit-border-horizontal-spacing: 0px; =
-webkit-border-vertical-spacing: 0px; =
-webkit-text-decorations-in-effect: none; -webkit-text-size-adjust: =
auto; -webkit-text-stroke-width: 0px; "><div style=3D"word-wrap: =
break-word; -webkit-nbsp-mode: space; -webkit-line-break: =
after-white-space; " class=3D"">-- thorpej</div></span>
</div>
<br class=3D""></body></html>=
--Apple-Mail=_E03DE96F-24E2-4AA7-8AB7-0027A868B57E--
From: mlelstv@serpens.de (Michael van Elst)
To: gnats-bugs@netbsd.org
Cc:
Subject: Re: kern/53217: -current: dumping to wedges on GPT disks broken
Date: Fri, 27 Apr 2018 15:45:28 -0000 (UTC)
martin@duskware.de (Martin Husemann) writes:
> > In dksubr.c dkdump (not the wedge code, the general disk support) then
> > examines as I quoted
> > 796 /* Convert to disk sectors. Request must be a
> > multiple of size. */
> > 797 part = DISKPART(dev);
> > ...
> > 806 p = &lp->d_partitions[part];
>
> And here accessing the label at all is clearly a bug.
It's what the code is supposed to do. Allow dumping only on
a swap partition (as in "disklabel partition").
For dumping on a wedge, you have to allow dumping to the
RAW partition. The driver's d_dump entry doesn't know wether
it's called from dumpsys() directly, some other pseudo disk
driver or from the wedge driver.
To avoid surprises you can check wether the RAW partition
is FS_UNUSED and wether the disk has actually defined wedges.
You could still:
- configure the raw partition as a dump device (there is no check).
- erroneously dump to the raw partition before wedges are discovered
or created.
--
--
Michael van Elst
Internet: mlelstv@serpens.de
"A potential Snark may lurk in every tree."
From: mlelstv@serpens.de (Michael van Elst)
To: gnats-bugs@netbsd.org
Cc:
Subject: Re: kern/53217: -current: dumping to wedges on GPT disks broken
Date: Fri, 27 Apr 2018 16:04:07 -0000 (UTC)
martin@duskware.de (Martin Husemann) writes:
> > In dksubr.c dkdump (not the wedge code, the general disk support) then
> > examines as I quoted
> > 796 /* Convert to disk sectors. Request must be a
> > multiple of size. */
> > 797 part = DISKPART(dev);
> > ...
> > 806 p = &lp->d_partitions[part];
>
> And here accessing the label at all is clearly a bug.
There is more to do, as the dump address / size is checked against
the partition size.
--
--
Michael van Elst
Internet: mlelstv@serpens.de
"A potential Snark may lurk in every tree."
Responsible-Changed-From-To: kern-bug-people->mlelstv
Responsible-Changed-By: mlelstv@NetBSD.org
Responsible-Changed-When: Sat, 28 Apr 2018 15:03:50 +0000
Responsible-Changed-Why:
working on a patch
State-Changed-From-To: open->analyzed
State-Changed-By: mlelstv@NetBSD.org
State-Changed-When: Sat, 28 Apr 2018 15:03:50 +0000
State-Changed-Why:
mine
From: mlelstv@serpens.de (Michael van Elst)
To: gnats-bugs@netbsd.org
Cc:
Subject: Re: kern/53217: -current: dumping to wedges on GPT disks broken
Date: Sat, 28 Apr 2018 15:02:28 -0000 (UTC)
kardel@netbsd.org writes:
>>Description:
> dumping on a swap wedge from a GPT partitioned disk fails with
> dump: device bad
>>Fix:
Maybe this:
Index: sys/dev/dksubr.c
===================================================================
RCS file: /cvsroot/src/sys/dev/dksubr.c,v
retrieving revision 1.101
diff -u -r1.101 dksubr.c
--- sys/dev/dksubr.c 4 Dec 2017 22:15:52 -0000 1.101
+++ sys/dev/dksubr.c 28 Apr 2018 14:55:39 -0000
@@ -762,10 +762,11 @@
daddr_t blkno, void *vav, size_t size)
{
const struct dkdriver *dkd = dksc->sc_dkdev.dk_driver;
+ struct disk_geom *dg = &dksc->sc_dkdev.dk_geom;
char *va = vav;
struct disklabel *lp;
struct partition *p;
- int part, towrt, nsects, sectoff, maxblkcnt, nblk;
+ int part, towrt, maxblkcnt, nblk;
int maxxfer, rv = 0;
/*
@@ -804,23 +805,44 @@
blkno = dbtob(blkno) / lp->d_secsize; /* blkno in secsize units */
p = &lp->d_partitions[part];
- if (p->p_fstype != FS_SWAP) {
- DPRINTF(DKDB_DUMP, ("%s: bad fstype %d\n", __func__,
- p->p_fstype));
- return ENXIO;
- }
- nsects = p->p_size;
- sectoff = p->p_offset;
+ if (part == RAW_PART) {
+ if (p->p_fstype != FS_UNUSED) {
+ DPRINTF(DKDB_DUMP, ("%s: bad fstype %d\n", __func__,
+ p->p_fstype));
+ return ENXIO;
+ }
+ /* Check wether dump goes to a wedge */
+ if (dksc->sc_dkdev.dk_nwedges == 0) {
+ DPRINTF(DKDB_DUMP, ("%s: dump to raw\n", __func__));
+ return ENXIO;
+ }
+ /* Check transfer bounds against media size */
+ if (blkno < 0 || (blkno + towrt) > dg->dg_secperunit) {
+ DPRINTF(DKDB_DUMP, ("%s: out of bounds blkno=%jd, towrt=%d, "
+ "nsects=%jd\n", __func__, (intmax_t)blkno, towrt, dg->dg_secperunit));
+ return EINVAL;
+ }
+ } else {
+ int nsects, sectoff;
- /* Check transfer bounds against partition size. */
- if ((blkno < 0) || ((blkno + towrt) > nsects)) {
- DPRINTF(DKDB_DUMP, ("%s: out of bounds blkno=%jd, towrt=%d, "
- "nsects=%d\n", __func__, (intmax_t)blkno, towrt, nsects));
- return EINVAL;
- }
+ if (p->p_fstype != FS_SWAP) {
+ DPRINTF(DKDB_DUMP, ("%s: bad fstype %d\n", __func__,
+ p->p_fstype));
+ return ENXIO;
+ }
+ nsects = p->p_size;
+ sectoff = p->p_offset;
- /* Offset block number to start of partition. */
- blkno += sectoff;
+ /* Check transfer bounds against partition size. */
+ if ((blkno < 0) || ((blkno + towrt) > nsects)) {
+ DPRINTF(DKDB_DUMP, ("%s: out of bounds blkno=%jd, towrt=%d, "
+ "nsects=%d\n", __func__, (intmax_t)blkno, towrt, nsects));
+ return EINVAL;
+ }
+
+ /* Offset block number to start of partition. */
+ blkno += sectoff;
+ }
/* Start dumping and return when done. */
maxblkcnt = howmany(maxxfer, lp->d_secsize);
Index: sys/dev/dkwedge/dk.c
===================================================================
RCS file: /cvsroot/src/sys/dev/dkwedge/dk.c,v
retrieving revision 1.96
diff -u -r1.96 dk.c
--- sys/dev/dkwedge/dk.c 5 Mar 2017 23:07:12 -0000 1.96
+++ sys/dev/dkwedge/dk.c 28 Apr 2018 14:55:39 -0000
@@ -1613,7 +1613,7 @@
rv = EINVAL;
goto out;
}
- if (blkno + size / DEV_BSIZE > sc->sc_size) {
+ if (blkno < 0 || blkno + size / DEV_BSIZE > sc->sc_size) {
printf("%s: blkno (%" PRIu64 ") + size / DEV_BSIZE (%zu) > "
"sc->sc_size (%" PRIu64 ")\n", __func__, blkno,
size / DEV_BSIZE, sc->sc_size);
--
--
Michael van Elst
Internet: mlelstv@serpens.de
"A potential Snark may lurk in every tree."
From: Frank Kardel <kardel@netbsd.org>
To: gnats-bugs@NetBSD.org
Cc:
Subject: Re: kern/53217: -current: dumping to wedges on GPT disks broken
Date: Sat, 28 Apr 2018 19:00:03 +0200
Looks fine to me - dumps to dk1 work in my environment. I have not tested
non GPT labeled system nor for error conditions - that is for somebody else.
Would we need pullups to -8 ?
Frank
On 04/28/18 17:05, Michael van Elst wrote:
> Index: sys/dev/dksubr.c
> ===================================================================
> RCS file: /cvsroot/src/sys/dev/dksubr.c,v
> retrieving revision 1.101
> diff -u -r1.101 dksubr.c
> --- sys/dev/dksubr.c 4 Dec 2017 22:15:52 -0000 1.101
> +++ sys/dev/dksubr.c 28 Apr 2018 14:55:39 -0000
> @@ -762,10 +762,11 @@
> daddr_t blkno, void *vav, size_t size)
> {
> const struct dkdriver *dkd = dksc->sc_dkdev.dk_driver;
> + struct disk_geom *dg = &dksc->sc_dkdev.dk_geom;
> char *va = vav;
> struct disklabel *lp;
> struct partition *p;
> - int part, towrt, nsects, sectoff, maxblkcnt, nblk;
> + int part, towrt, maxblkcnt, nblk;
> int maxxfer, rv = 0;
>
> /*
> @@ -804,23 +805,44 @@
> blkno = dbtob(blkno) / lp->d_secsize; /* blkno in secsize units */
>
> p = &lp->d_partitions[part];
> - if (p->p_fstype != FS_SWAP) {
> - DPRINTF(DKDB_DUMP, ("%s: bad fstype %d\n", __func__,
> - p->p_fstype));
> - return ENXIO;
> - }
> - nsects = p->p_size;
> - sectoff = p->p_offset;
> + if (part == RAW_PART) {
> + if (p->p_fstype != FS_UNUSED) {
> + DPRINTF(DKDB_DUMP, ("%s: bad fstype %d\n", __func__,
> + p->p_fstype));
> + return ENXIO;
> + }
> + /* Check wether dump goes to a wedge */
> + if (dksc->sc_dkdev.dk_nwedges == 0) {
> + DPRINTF(DKDB_DUMP, ("%s: dump to raw\n", __func__));
> + return ENXIO;
> + }
> + /* Check transfer bounds against media size */
> + if (blkno < 0 || (blkno + towrt) > dg->dg_secperunit) {
> + DPRINTF(DKDB_DUMP, ("%s: out of bounds blkno=%jd, towrt=%d, "
> + "nsects=%jd\n", __func__, (intmax_t)blkno, towrt, dg->dg_secperunit));
> + return EINVAL;
> + }
> + } else {
> + int nsects, sectoff;
>
> - /* Check transfer bounds against partition size. */
> - if ((blkno < 0) || ((blkno + towrt) > nsects)) {
> - DPRINTF(DKDB_DUMP, ("%s: out of bounds blkno=%jd, towrt=%d, "
> - "nsects=%d\n", __func__, (intmax_t)blkno, towrt, nsects));
> - return EINVAL;
> - }
> + if (p->p_fstype != FS_SWAP) {
> + DPRINTF(DKDB_DUMP, ("%s: bad fstype %d\n", __func__,
> + p->p_fstype));
> + return ENXIO;
> + }
> + nsects = p->p_size;
> + sectoff = p->p_offset;
>
> - /* Offset block number to start of partition. */
> - blkno += sectoff;
> + /* Check transfer bounds against partition size. */
> + if ((blkno < 0) || ((blkno + towrt) > nsects)) {
> + DPRINTF(DKDB_DUMP, ("%s: out of bounds blkno=%jd, towrt=%d, "
> + "nsects=%d\n", __func__, (intmax_t)blkno, towrt, nsects));
> + return EINVAL;
> + }
> +
> + /* Offset block number to start of partition. */
> + blkno += sectoff;
> + }
>
> /* Start dumping and return when done. */
> maxblkcnt = howmany(maxxfer, lp->d_secsize);
> Index: sys/dev/dkwedge/dk.c
> ===================================================================
> RCS file: /cvsroot/src/sys/dev/dkwedge/dk.c,v
> retrieving revision 1.96
> diff -u -r1.96 dk.c
> --- sys/dev/dkwedge/dk.c 5 Mar 2017 23:07:12 -0000 1.96
> +++ sys/dev/dkwedge/dk.c 28 Apr 2018 14:55:39 -0000
> @@ -1613,7 +1613,7 @@
> rv = EINVAL;
> goto out;
> }
> - if (blkno + size / DEV_BSIZE > sc->sc_size) {
> + if (blkno < 0 || blkno + size / DEV_BSIZE > sc->sc_size) {
> printf("%s: blkno (%" PRIu64 ") + size / DEV_BSIZE (%zu) > "
> "sc->sc_size (%" PRIu64 ")\n", __func__, blkno,
> size / DEV_BSIZE, sc->sc_size);
>
>
From: John Nemeth <jnemeth@cue.bc.ca>
To: gnats-bugs@NetBSD.org, kern-bug-people@netbsd.org, gnats-admin@netbsd.org,
netbsd-bugs@netbsd.org, kardel@netbsd.org
Cc:
Subject: Re: kern/53217: -current: dumping to wedges on GPT disks broken
Date: Thu, 3 May 2018 11:26:24 -0700
On Apr 27, 7:45am, Martin Husemann wrote:
}
} The following reply was made to PR kern/53217; it has been noted by GNATS.
}
} From: Martin Husemann <martin@duskware.de>
} To: gnats-bugs@NetBSD.org
} Cc:
} Subject: Re: kern/53217: -current: dumping to wedges on GPT disks broken
} Date: Fri, 27 Apr 2018 09:42:05 +0200
}
} On Thu, Apr 26, 2018 at 07:10:00PM +0000, kardel@netbsd.org wrote:
} > >Fix:
} > Maybe also accept FS_UNUSED as fstype?
}
} I'd rather have the dkwedge_gpt.c code set the partition type of the
} fake label for the whole disk of the wedge to FS_SWAP when dkw.dkw_ptype
} is DKW_PTYPE_SWAP.
Should we be migrating away from the use of labels as an API
and have them as only a low level detail that gets buried and only
seen by utility apps? We already have DIOCGSECTORSIZE and
DIOCGMEDIASIZE. Maybe we need something like DIOCGPARTTYPE as a
generic way to get a partition type without needing to know anything
about the underlying partition scheme?
}-- End of excerpt from Martin Husemann
From: mlelstv@serpens.de (Michael van Elst)
To: gnats-bugs@netbsd.org
Cc:
Subject: Re: kern/53217: -current: dumping to wedges on GPT disks broken
Date: Fri, 4 May 2018 10:25:36 -0000 (UTC)
jnemeth@cue.bc.ca (John Nemeth) writes:
> Should we be migrating away from the use of labels as an API
>and have them as only a low level detail that gets buried and only
>seen by utility apps? We already have DIOCGSECTORSIZE and
>DIOCGMEDIASIZE. Maybe we need something like DIOCGPARTTYPE as a
>generic way to get a partition type without needing to know anything
>about the underlying partition scheme?
That would be methods to move this information to userland. If you only
want wedges, you already have DIOCGWEDGEINFO that returns the partition
type.
The kernel has the utility functions getdisksize() and getdiskinfo() that
hide such details.
This has little to do with the kernel dumps. The dump (and size) vectors
are ancient calls into the disk driver to allow dumps even when the
kernel has crashed. Before touching this, I'd first make wedges a first
class citizen and make them the default for installations, which mostly
requires DKWEDGE_METHOD_DISKLABEL and some easy transition plan for
users.
--
--
Michael van Elst
Internet: mlelstv@serpens.de
"A potential Snark may lurk in every tree."
State-Changed-From-To: analyzed->closed
State-Changed-By: kardel@NetBSD.org
State-Changed-When: Mon, 03 Jun 2019 06:46:07 +0000
State-Changed-Why:
dumping to wedges seems to work right now.
>Unformatted:
(Contact us)
$NetBSD: query-full-pr,v 1.46 2020/01/03 16:35:01 leot Exp $
$NetBSD: gnats_config.sh,v 1.9 2014/08/02 14:16:04 spz Exp $
Copyright © 1994-2020
The NetBSD Foundation, Inc. ALL RIGHTS RESERVED.
Home