Bug 141235 - [geom_part] 8.0 no longer provides /dev entries for all disk slices [regression]
Summary: [geom_part] 8.0 no longer provides /dev entries for all disk slices [regression]
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: Unspecified
Hardware: Any Any
: Normal Affects Only Me
Assignee: Andrey V. Elsukov
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-12-07 02:10 UTC by Dieter
Modified: 2011-07-27 05:30 UTC (History)
0 users

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Dieter 2009-12-07 02:10:04 UTC
FreeBSD 8.0 RELEASE for amd64 does not provide /dev entries for
disk slices above 4. Slices greater than 4 work fine in 6.x and 7.x

Example, 7.1 amd64:

ls /dev/ad4*
/dev/ad4        /dev/ad4s2      /dev/ad4s2d     /dev/ad4s5      /dev/ad4s5d     /dev/ad4s6
/dev/ad4s1      /dev/ad4s2a     /dev/ad4s2e     /dev/ad4s5a     /dev/ad4s5e     /dev/ad4s7
/dev/ad4s10     /dev/ad4s2b     /dev/ad4s3      /dev/ad4s5b     /dev/ad4s5f     /dev/ad4s8
/dev/ad4s11     /dev/ad4s2c     /dev/ad4s4      /dev/ad4s5c     /dev/ad4s5g     /dev/ad4s9

11 slices and they all work fine.  Some slices are further subdivided
with BSD disklabels, some are not.  They work fine either way.

But 8.0 only provides /dev entries for slices 1-4.  Slices 5 and above
cannot be accessed.

FreeBSD 8 needs to provide /dev entries for all the slices.

How-To-Repeat: Setup a disk with more than 4 slices.  Boot 6.x or 7.x and observe that
they all work.  Boot 8.0 and observe that there are no /dev entries
above slice 4.
Comment 1 Mark Linimon freebsd_committer freebsd_triage 2009-12-07 07:22:03 UTC
Responsible Changed
From-To: freebsd-bugs->freebsd-fs

Over to maintainer(s). 

To submitter: does this happen to be a 'dangerously dedicated' disk?
Comment 2 Mark Linimon freebsd_committer freebsd_triage 2010-02-05 06:33:28 UTC
Responsible Changed
From-To: freebsd-fs->freebsd-bugs

On looking at this again, I'm wondering if it might not be a problem 
with the ata rework, instead.
Comment 3 Jaakko Heinonen freebsd_committer freebsd_triage 2010-04-30 08:42:16 UTC
State Changed
From-To: open->feedback

This is probably related to GEOM_PART_*. Could you confirm that by replacing 
GEOM_PART_* options in your kernel config with GEOM_MBR and GEOM_BSD?
Comment 4 Andrey V. Elsukov 2010-04-30 09:02:38 UTC
Hi,

Can you show output of `gpart show` and `kldstat -v | grep part`?

-- 
WBR, Andrey V. Elsukov
Comment 5 dieterbsd 2010-05-27 23:51:00 UTC
> This is probably related to GEOM_PART_*. Could you confirm that by=20
replacing
> GEOM_PART_* options in your kernel config with GEOM_MBR and GEOM_BSD?

opt_geom.h previously contained:

#define GEOM_LABEL 1
#define GEOM_PART_EBR 1
#define GEOM_PART_GPT 1
#define GEOM_PART_MBR 1
#define GEOM_PART_EBR_COMPAT 1
#define GEOM_PART_BSD 1

Added GEOM_MBR and GEOM_BSD to config file.
(previous config file was stock GENERIC)

opt_geom.h now:

#define GEOM_BSD 1
#define GEOM_LABEL 1
#define GEOM_PART_EBR 1
#define GEOM_PART_GPT 1
#define GEOM_PART_MBR 1
#define GEOM_PART_EBR_COMPAT 1
#define GEOM_PART_BSD 1
#define GEOM_MBR 1

7.1 kernel works properly:

ls -l /dev/ad4*
crw-r-----  1 root  operator    0,  85 May 27 19:24 /dev/ad4
crw-r-----  1 root  operator    0,  89 May 27 19:24 /dev/ad4s1
crw-r-----  1 root  operator    0,  90 May 27 19:24 /dev/ad4s2
crw-r-----  1 root  operator    0,  91 May 27 19:24 /dev/ad4s3
crw-r-----  1 root  operator    0,  98 May 27 19:24 /dev/ad4s3a
crw-r-----  1 root  operator    0,  99 May 27 19:24 /dev/ad4s3c
crw-r-----  1 root  operator    0,  92 May 27 19:24 /dev/ad4s4
crw-r-----  1 root  operator    0, 100 May 27 19:24 /dev/ad4s4a
crw-r-----  1 root  operator    0, 101 May 27 19:24 /dev/ad4s4b
crw-r-----  1 root  operator    0, 102 May 27 19:24 /dev/ad4s4c

ls -l /dev/ad20*
crw-r-----  1 root  operator    0, 120 May 27 19:24 /dev/ad20
crw-r-----  1 root  operator    0, 163 May 27 19:24 /dev/ad20s1
crw-r-----  1 root  operator    0, 164 May 27 19:24 /dev/ad20s2
crw-r-----  1 root  operator    0, 165 May 27 19:24 /dev/ad20s3
crw-r-----  1 root  operator    0, 166 May 27 19:24 /dev/ad20s4
crw-r-----  1 root  operator    0, 194 May 27 19:24 /dev/ad20s5        =20
extended partition
#

-------------------------

stock 8.0 kernel:

(ada4 in 8.0 is same disk as ad20 in 7.1)

ls -l /dev/ad4*
crw-r-----  1 root  operator    0, 102 May 27 12:13 /dev/ad4
crw-r-----  1 root  operator    0, 103 May 27 12:13 /dev/ad4s1
crw-r-----  1 root  operator    0, 104 May 27 12:13 /dev/ad4s2
crw-r-----  1 root  operator    0, 105 May 27 12:13 /dev/ad4s3
crw-r-----  1 root  operator    0, 109 May 27 12:13 /dev/ad4s3a
crw-r-----  1 root  operator    0, 106 May 27 12:13 /dev/ad4s4
crw-r-----  1 root  operator    0, 110 May 27 12:13 /dev/ad4s4a
crw-r-----  1 root  operator    0, 111 May 27 12:13 /dev/ad4s4b

ls -l /dev/ada4*
crw-r-----  1 root  operator    0, 180 May 27 12:13 /dev/ada4
crw-r-----  1 root  operator    0, 223 May 27 12:13 /dev/ada4s1
crw-r-----  1 root  operator    0, 224 May 27 12:13 /dev/ada4s2
crw-r-----  1 root  operator    0, 225 May 27 12:13 /dev/ada4s3
crw-r-----  1 root  operator    0, 226 May 27 12:13 /dev/ada4s4

/dev/ada4s5 is missing

----------

8.0 with GEOM_MBR and GEOM_BSD:

ls -l /dev/ad4*
crw-r-----  1 root  operator    0, 106 May 27 12:02 /dev/ad4
crw-r-----  1 root  operator    0, 116 May 27 12:02 /dev/ad4s1
crw-r-----  1 root  operator    0, 117 May 27 12:02 /dev/ad4s2
crw-r-----  1 root  operator    0, 118 May 27 12:02 /dev/ad4s3
crw-r-----  1 root  operator    0, 131 May 27 12:02 /dev/ad4s3a
crw-r-----  1 root  operator    0, 131 May 27 12:02 /dev/ad4s3a        =20
dup ?
crw-r-----  1 root  operator    0, 132 May 27 12:02 /dev/ad4s3c
crw-r-----  1 root  operator    0, 161 May 27 12:02 /dev/ad4s3ca       =20
?
crw-r-----  1 root  operator    0, 119 May 27 12:02 /dev/ad4s4
crw-r-----  1 root  operator    0, 134 May 27 12:02 /dev/ad4s4a
crw-r-----  1 root  operator    0, 135 May 27 12:02 /dev/ad4s4b
crw-r-----  1 root  operator    0, 136 May 27 12:02 /dev/ad4s4c
crw-r-----  1 root  operator    0, 166 May 27 12:02 /dev/ad4s4ca       =20
?
crw-r-----  1 root  operator    0, 167 May 27 12:02 /dev/ad4s4cb       =20
?

ls -l /dev/ada4* | cat -v
crw-r-----  1 root  operator    1,  29 May 27 12:02 /dev/ada4
crw-r-----  1 root  operator    1,  72 May 27 12:02 /dev/ada4s1
crw-r-----  1 root  operator    1,  72 May 27 12:02 /dev/ada4s1        =20
dup ?
crw-r-----  1 root  operator    1,  73 May 27 12:02 /dev/ada4s2
crw-r-----  1 root  operator    1,  73 May 27 12:02 /dev/ada4s2        =20
dup ?
crw-r-----  1 root  operator    1,  74 May 27 12:02 /dev/ada4s3
crw-r-----  1 root  operator    1,  74 May 27 12:02 /dev/ada4s3        =20
dup ?
crw-r-----  1 root  operator    1,  75 May 27 12:02 /dev/ada4s4
crw-r-----  1 root  operator    1,  75 May 27 12:02 /dev/ada4s4        =20
dup ?
crw-r-----  1 root  operator    1, 193 May 27 12:02 /dev/ada4s4s1      =20
?
crw-r-----  1 root  operator    1, 193 May 27 12:02 /dev/ada4s4s1      =20
?
crw-r-----  1 root  operator    1, 192 May 27 12:02 /dev/ada4s5        =20
extended partition showed up

So the extended partition showed up, but now I have some duplicate /dev=20
entries
and some oddball bogus entries.
Comment 6 Jaakko Heinonen freebsd_committer freebsd_triage 2010-05-28 10:52:02 UTC
On 2010-05-27, dieterbsd@engineer.com wrote:
> opt_geom.h previously contained:
> 
> #define GEOM_LABEL 1
> #define GEOM_PART_EBR 1
> #define GEOM_PART_GPT 1
> #define GEOM_PART_MBR 1
> #define GEOM_PART_EBR_COMPAT 1
> #define GEOM_PART_BSD 1
> 
> Added GEOM_MBR and GEOM_BSD to config file.
> (previous config file was stock GENERIC)

> opt_geom.h now:
> 
> #define GEOM_BSD 1
> #define GEOM_LABEL 1
> #define GEOM_PART_EBR 1
> #define GEOM_PART_GPT 1
> #define GEOM_PART_MBR 1
> #define GEOM_PART_EBR_COMPAT 1
> #define GEOM_PART_BSD 1
> #define GEOM_MBR 1

You should remove GEOM_PART_* entries. I think that the duplicates you
saw are because you had geom_part still enabled.

> 8.0 with GEOM_MBR and GEOM_BSD:
[...]
> So the extended partition showed up, but now I have some duplicate /dev 
> entries
> and some oddball bogus entries.

-- 
Jaakko
Comment 7 dieterbsd 2010-05-28 16:53:49 UTC
> You should remove GEOM_PART_* entries. I think that the duplicates you
> saw are because you had geom_part still enabled.

I thought about that.  Having both GEOM_PART_MBR and GEOM_MBR
seemed suspicious, but GEOM_PART_MBR isn't from the config file:

grep -i geom GENERIC
options         GEOM_PART_GPT           # GUID Partition Tables.
options         GEOM_LABEL              # Provides labelization

and then I added GEOM_MBR and GEOM_BSD as suggested.  So I don't
know where GEOM_PART_MBR comes from or how to get rid of it.
I am assuming that "make buildkernel" calls config(8) and config
builds opt_geom.h based on the config file, but it must have some
other input that I haven't found.

It isn't clear to me what the difference between GEOM_PART_MBR and
GEOM_MBR is supposed to be (same for GEOM_PART_BSD and GEOM_BSD).
conf/NOTES doesn't list a GEOM_GPT, only GEOM_PART_GPT.  I assume
that removing GEOM_PART_GPT would break disks using GUID partition
tables.
Comment 8 Garrett Cooper 2010-05-28 18:07:01 UTC
On Fri, May 28, 2010 at 8:53 AM,  <dieterbsd@engineer.com> wrote:
>> You should remove GEOM_PART_* entries. I think that the duplicates you
>> saw are because you had geom_part still enabled.
>
> I thought about that. =A0Having both GEOM_PART_MBR and GEOM_MBR
> seemed suspicious, but GEOM_PART_MBR isn't from the config file:
>
> grep -i geom GENERIC
> options =A0 =A0 =A0 =A0 GEOM_PART_GPT =A0 =A0 =A0 =A0 =A0 # GUID Partitio=
n Tables.
> options =A0 =A0 =A0 =A0 GEOM_LABEL =A0 =A0 =A0 =A0 =A0 =A0 =A0# Provides =
labelization
>
> and then I added GEOM_MBR and GEOM_BSD as suggested. =A0So I don't
> know where GEOM_PART_MBR comes from or how to get rid of it.
> I am assuming that "make buildkernel" calls config(8) and config
> builds opt_geom.h based on the config file, but it must have some
> other input that I haven't found.
>
> It isn't clear to me what the difference between GEOM_PART_MBR and
> GEOM_MBR is supposed to be (same for GEOM_PART_BSD and GEOM_BSD).
> conf/NOTES doesn't list a GEOM_GPT, only GEOM_PART_GPT. =A0I assume
> that removing GEOM_PART_GPT would break disks using GUID partition
> tables.

GEOM_MBR is the legacy way to specify GEOM_PART_MBR IIRC (from .../UPDATING=
):

20090320:
        GEOM_PART has become the default partition slicer for storage devic=
es,
        replacing GEOM_MBR, GEOM_BSD, GEOM_PC98 and GEOM_GPT slicers. It
        introduces some changes:

        MSDOS/EBR: the devices created from MSDOS extended partition entrie=
s
        (EBR) can be named differently than with GEOM_MBR and are now symli=
nks
        to devices with offset-based names. fstabs may need to be modified.

        BSD: the "geometry does not match label" warning is harmless in mos=
t
        cases but it points to problems in file system misalignment with
        disk geometry. The "c" partition is now implicit, covers the whole
        top-level drive and cannot be (mis)used by users.

        General: Kernel dumps are now not allowed to be written to devices
        whose partition types indicate they are meant to be used for file
        systems (or, in case of MSDOS partitions, as something else than
        the "386BSD" type).

        Most of these changes date approximately from 200812.

Thanks,
-Garrett
Comment 9 Garrett Cooper 2010-05-28 18:18:50 UTC
On Fri, May 28, 2010 at 10:07 AM, Garrett Cooper <yanefbsd@gmail.com> wrote=
:
> On Fri, May 28, 2010 at 8:53 AM, =A0<dieterbsd@engineer.com> wrote:
>>> You should remove GEOM_PART_* entries. I think that the duplicates you
>>> saw are because you had geom_part still enabled.
>>
>> I thought about that. =A0Having both GEOM_PART_MBR and GEOM_MBR
>> seemed suspicious, but GEOM_PART_MBR isn't from the config file:
>>
>> grep -i geom GENERIC
>> options =A0 =A0 =A0 =A0 GEOM_PART_GPT =A0 =A0 =A0 =A0 =A0 # GUID Partiti=
on Tables.
>> options =A0 =A0 =A0 =A0 GEOM_LABEL =A0 =A0 =A0 =A0 =A0 =A0 =A0# Provides=
 labelization
>>
>> and then I added GEOM_MBR and GEOM_BSD as suggested. =A0So I don't
>> know where GEOM_PART_MBR comes from or how to get rid of it.
>> I am assuming that "make buildkernel" calls config(8) and config
>> builds opt_geom.h based on the config file, but it must have some
>> other input that I haven't found.
>>
>> It isn't clear to me what the difference between GEOM_PART_MBR and
>> GEOM_MBR is supposed to be (same for GEOM_PART_BSD and GEOM_BSD).
>> conf/NOTES doesn't list a GEOM_GPT, only GEOM_PART_GPT. =A0I assume
>> that removing GEOM_PART_GPT would break disks using GUID partition
>> tables.
>
> GEOM_MBR is the legacy way to specify GEOM_PART_MBR IIRC (from .../UPDATI=
NG):
>
> 20090320:
> =A0 =A0 =A0 =A0GEOM_PART has become the default partition slicer for stor=
age devices,
> =A0 =A0 =A0 =A0replacing GEOM_MBR, GEOM_BSD, GEOM_PC98 and GEOM_GPT slice=
rs. It
> =A0 =A0 =A0 =A0introduces some changes:
>
> =A0 =A0 =A0 =A0MSDOS/EBR: the devices created from MSDOS extended partiti=
on entries
> =A0 =A0 =A0 =A0(EBR) can be named differently than with GEOM_MBR and are =
now symlinks
> =A0 =A0 =A0 =A0to devices with offset-based names. fstabs may need to be =
modified.
>
> =A0 =A0 =A0 =A0BSD: the "geometry does not match label" warning is harmle=
ss in most
> =A0 =A0 =A0 =A0cases but it points to problems in file system misalignmen=
t with
> =A0 =A0 =A0 =A0disk geometry. The "c" partition is now implicit, covers t=
he whole
> =A0 =A0 =A0 =A0top-level drive and cannot be (mis)used by users.
>
> =A0 =A0 =A0 =A0General: Kernel dumps are now not allowed to be written to=
 devices
> =A0 =A0 =A0 =A0whose partition types indicate they are meant to be used f=
or file
> =A0 =A0 =A0 =A0systems (or, in case of MSDOS partitions, as something els=
e than
> =A0 =A0 =A0 =A0the "386BSD" type).
>
> =A0 =A0 =A0 =A0Most of these changes date approximately from 200812.

I did a bit more digging and what I discovered wasn't entirely true in
my first statement; yes, GEOM_MBR is the legacy system from first
glance compared to GEOM_PART_MBR, but that doesn't tell the entire
story. The two items (GEOM_PART_MBR and GEOM_MBR) also grossly differ
with one another.

I'm going to take a look at the code in the meantime and try to better
digest what's going on (because it would help with work), but just for
future reference the files that get used [up through 9-CURRENT] are
located at:

/sys/geom/geom_mbr.c
/sys/geom/part/g_part_mbr.c

You can of course determine this info from /sys/conf/files.
HTH,
-Garrett
Comment 10 Jaakko Heinonen freebsd_committer freebsd_triage 2010-05-30 14:51:40 UTC
On 2010-05-28, dieterbsd@engineer.com wrote:
> and then I added GEOM_MBR and GEOM_BSD as suggested.  So I don't
> know where GEOM_PART_MBR comes from or how to get rid of it.

GEOM_PART_* are in DEFAULTS (sys/amd64/conf/DEFAULTS on amd64).

-- 
Jaakko
Comment 11 dieterbsd 2010-05-30 21:56:56 UTC
>> and then I added GEOM_MBR and GEOM_BSD as suggested.  So I don't
>> know where GEOM_PART_MBR comes from or how to get rid of it.
>
> GEOM_PART_* are in DEFAULTS (sys/amd64/conf/DEFAULTS on amd64).

Ah ha!

opt_geom.h now contains:

#define GEOM_BSD 1
#define GEOM_LABEL 1
#define GEOM_PART_GPT 1
#define GEOM_MBR 1

(same as 7.1 had)

Now I have /dev entries for extended partitions, and the
duplicate and bogus entries are gone.  GUID partitioning
and partition labels still work.  Yea!

Having some GEOM options in DEFAULTS and some in the config file
seems prone to error.  I suggest putting them all in the same file.
Yes, grep can easily find it, but despite many years of experience
with grep, find, and xargs I somehow managed to miss DEFAULTS.
I can only blame lack of sleep.

Having both GEOM_MBR and GEOM_PART_MBR is confusing.  If there is a
good reason to retain both, better documentation might be in order.
Same for GEOM_BSD and GEOM_PART_BSD.

Thank you.
Comment 12 Jaakko Heinonen freebsd_committer freebsd_triage 2010-05-31 17:41:23 UTC
State Changed
From-To: feedback->open

Feedback received. 


Comment 13 Jaakko Heinonen freebsd_committer freebsd_triage 2010-05-31 17:41:23 UTC
Responsible Changed
From-To: freebsd-bugs->freebsd-geom

Confirmed to be a geom_part problem.
Comment 14 Andrey V. Elsukov 2010-05-31 18:11:54 UTC
Hi,

Did you try to remove both deprecated GEOM_BSD and GEOM_MBR
from your config?
Now in 8.0+ you should use GEOM_PART_MBR, GEOM_PART_EBR and
GEOM_PART_BSD instead. And they all are in DEFAULTS config by
default.

Your MBR partitions should be served with GEOM_PART_MBR scheme.
An extended partitions should be served with GEOM_PART_EBR scheme.
A BSD slices should be served with GEOM_PART_BSD scheme.

-- 
WBR, Andrey V. Elsukov
Comment 15 dieterbsd 2010-06-03 19:30:26 UTC
> Did you try to remove both deprecated GEOM_BSD and GEOM_MBR
> from your config?
> Now in 8.0+ you should use GEOM_PART_MBR, GEOM_PART_EBR and
> GEOM_PART_BSD instead. And they all are in DEFAULTS config by
> default.
>
> Your MBR partitions should be served with GEOM_PART_MBR scheme.
> An extended partitions should be served with GEOM_PART_EBR scheme.
> A BSD slices should be served with GEOM_PART_BSD scheme.

I started with the default GENERIC 8.0-release kernel and it is broken.
It does not provide /dev entries for MBR extended partitions.

Changing the GEOM_* options to be

#define GEOM_BSD 1
#define GEOM_LABEL 1
#define GEOM_PART_GPT 1
#define GEOM_MBR 1

(which turns out to be the same as they were in 7.1) makes everything=20
work.
Comment 16 Andrey V. Elsukov 2010-06-05 09:25:55 UTC
Hi,

Can you try this patch to g_part_ebr.c to GENERIC kernel and
report results back?
http://svn.freebsd.org/viewvc/base?view=revision&revision=197608

-- 
WBR, Andrey V. Elsukov
Comment 17 dieterbsd 2010-06-07 01:15:48 UTC
> Can you try this patch to g_part_ebr.c to GENERIC kernel and
> report results back?

I reverted the GEOM_* options back to 8.0-release defaults, dropped
in the new g_part_ebr.c and now I get /dev entries for MBR extended
partitions.

So the

<       for (index =3D 0; index < DOSPARTOFF - 9; index++)
---
>       for (index =3D 96; index < DOSPARTOFF - 9; index++)

change fixed the problem.

Thanks.
Comment 18 Jaakko Heinonen freebsd_committer freebsd_triage 2010-06-07 19:39:22 UTC
State Changed
From-To: open->patched

Patched in head (r197608).
Comment 19 dfilter service freebsd_committer freebsd_triage 2010-06-07 21:32:11 UTC
Author: ae
Date: Mon Jun  7 20:31:55 2010
New Revision: 208899
URL: http://svn.freebsd.org/changeset/base/208899

Log:
  MFC r197608:
  
  The first 96 bytes may not be zeroes. It can contain trivial boot
  code that merely emits an error and waits for a key press before
  rebooting. The error being that extended partitions are not
  bootable. The origin is presumed to be Windows 2000; Windows XP
  does not do this...
  
  For now, ignore the first 96 bytes when checking that the EBR is
  (for the most part) all zeroes.
  
  Tested by:	Mario Lobo <mlobo at digiart.art.br>
  		Dieter <dieterbsd at engineer.com>
  PR:		kern/141235
  Reviewed by:	marcel
  Approved by:	kib (mentor)
  Approved by:	re (bz)

Modified:
  stable/8/sys/geom/part/g_part_ebr.c
Directory Properties:
  stable/8/sys/   (props changed)
  stable/8/sys/amd64/include/xen/   (props changed)
  stable/8/sys/cddl/contrib/opensolaris/   (props changed)
  stable/8/sys/contrib/dev/acpica/   (props changed)
  stable/8/sys/contrib/pf/   (props changed)
  stable/8/sys/dev/xen/xenpci/   (props changed)
  stable/8/sys/geom/sched/   (props changed)

Modified: stable/8/sys/geom/part/g_part_ebr.c
==============================================================================
--- stable/8/sys/geom/part/g_part_ebr.c	Mon Jun  7 18:47:53 2010	(r208898)
+++ stable/8/sys/geom/part/g_part_ebr.c	Mon Jun  7 20:31:55 2010	(r208899)
@@ -410,13 +410,13 @@ g_part_ebr_probe(struct g_part_table *ta
 		goto out;
 
 	/*
-	 * The sector is all zeroes, except for the partition entries
-	 * and some signatures or disk serial number. Those can be
-	 * found in the 9 bytes immediately in front of the partition
-	 * table.
+	 * The sector is all zeroes, except for the partition entries,
+	 * pseudo boot code and some signatures or disk serial number.
+	 * The latter can be found in the 9 bytes immediately in front
+	 * of the partition table.
 	 */
 	sum = 0;
-	for (index = 0; index < DOSPARTOFF - 9; index++)
+	for (index = 96; index < DOSPARTOFF - 9; index++)
 		sum += buf[index];
 	if (sum != 0)
 		goto out;
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org"
Comment 20 Andrey V. Elsukov freebsd_committer freebsd_triage 2010-07-10 18:09:51 UTC
State Changed
From-To: patched->suspended

Better solution needed.
http://www.freebsd.org/cgi/query-pr.cgi?pr=141235 
Comment 21 Andrey V. Elsukov freebsd_committer freebsd_triage 2010-12-21 10:42:39 UTC
State Changed
From-To: suspended->analyzed

Reopen this PR since I've got several similar reports and 
i have time to work on it. 


Comment 22 Andrey V. Elsukov freebsd_committer freebsd_triage 2010-12-21 10:42:39 UTC
Responsible Changed
From-To: freebsd-geom->ae

Reopen this PR since I've got several similar reports and 
i have time to work on it.
Comment 23 dfilter service freebsd_committer freebsd_triage 2010-12-28 08:36:50 UTC
Author: ae
Date: Tue Dec 28 08:36:44 2010
New Revision: 216754
URL: http://svn.freebsd.org/changeset/base/216754

Log:
  Make EBR probe method less strictly to be able detect EBRs with
  small non fatal inconsistency. EBR may contain boot loader and sometimes
  it just has some garbage data. Now this does not prevent FreeBSD to use
  extended partitions. But since we do not support bootcode for EBR we mark
  tables which have non empty boot area as corrupt. This does make them
  readonly and we can not damage this data.
  
  PR:		kern/141235
  MFC after:	1 month

Modified:
  head/sys/geom/part/g_part_ebr.c

Modified: head/sys/geom/part/g_part_ebr.c
==============================================================================
--- head/sys/geom/part/g_part_ebr.c	Tue Dec 28 03:27:20 2010	(r216753)
+++ head/sys/geom/part/g_part_ebr.c	Tue Dec 28 08:36:44 2010	(r216754)
@@ -377,7 +377,7 @@ g_part_ebr_probe(struct g_part_table *ta
 	char psn[8];
 	struct g_provider *pp;
 	u_char *buf, *p;
-	int error, index, res, sum;
+	int error, index, res;
 	uint16_t magic;
 
 	pp = cp->provider;
@@ -409,29 +409,11 @@ g_part_ebr_probe(struct g_part_table *ta
 	if (magic != DOSMAGIC)
 		goto out;
 
-	/*
-	 * The sector is all zeroes, except for the partition entries,
-	 * pseudo boot code and some signatures or disk serial number.
-	 * The latter can be found in the 9 bytes immediately in front
-	 * of the partition table.
-	 */
-	sum = 0;
-	for (index = 96; index < DOSPARTOFF - 9; index++)
-		sum += buf[index];
-	if (sum != 0)
-		goto out;
-
-	for (index = 0; index < NDOSPART; index++) {
+	for (index = 0; index < 2; index++) {
 		p = buf + DOSPARTOFF + index * DOSPARTSIZE;
 		if (p[0] != 0 && p[0] != 0x80)
 			goto out;
-		if (index < 2)
-			continue;
-		/* The 3rd & 4th entries are always zero. */
-		if ((le64dec(p+0) + le64dec(p+8)) != 0)
-			goto out;
 	}
-
 	res = G_PART_PROBE_PRI_NORM;
 
  out:
@@ -450,7 +432,7 @@ g_part_ebr_read(struct g_part_table *bas
 	u_char *buf;
 	off_t ofs, msize;
 	u_int lba;
-	int error, index;
+	int error, index, sum;
 
 	pp = cp->provider;
 	table = (struct g_part_ebr_table *)basetable;
@@ -465,6 +447,28 @@ g_part_ebr_read(struct g_part_table *bas
 
 		ebr_entry_decode(buf + DOSPARTOFF + 0 * DOSPARTSIZE, ent + 0);
 		ebr_entry_decode(buf + DOSPARTOFF + 1 * DOSPARTSIZE, ent + 1);
+
+		/* The 3rd & 4th entries should be zeroes. */
+		if (le64dec(buf + DOSPARTOFF + 2 * DOSPARTSIZE) +
+		    le64dec(buf + DOSPARTOFF + 3 * DOSPARTSIZE) != 0) {
+			basetable->gpt_corrupt = 1;
+			printf("GEOM: %s: invalid entries in the EBR ignored.\n",
+			    pp->name);
+		}
+		/* We do not support bootcode for EBR. If bootcode area is
+		 * not zeroes, then mark this EBR as corrupt to do not break
+		 * anything for another OS'es.
+		 */
+		if (lba == 0) {
+			sum = 0;
+			for (index = 0; index < DOSPARTOFF; index++)
+				sum += buf[index];
+			if (sum != 0) {
+				basetable->gpt_corrupt = 1;
+				printf("GEOM: %s: EBR has non empty bootcode.\n",
+				    pp->name);
+			}
+		}
 		g_free(buf);
 
 		if (ent[0].dp_typ == 0)
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org"
Comment 24 Andrey V. Elsukov freebsd_committer freebsd_triage 2010-12-28 09:15:24 UTC
State Changed
From-To: analyzed->patched

Patched in head/.
Comment 25 Niclas Zeising 2011-05-17 14:22:21 UTC
There has been plenty more time than the 1 month MFC timeout. This can
be merged into 8 and the pr closed.
Regards!
-- 
Niclas
Comment 26 Andrey V. Elsukov 2011-05-17 15:15:26 UTC
On 17.05.2011 17:22, Niclas Zeising wrote:
> There has been plenty more time than the 1 month MFC timeout. This can
> be merged into 8 and the pr closed.
> Regards!


I have work in progress that is related to these changes and i want do
MFC after it will be commited.

-- 
WBR, Andrey V. Elsukov
Comment 27 dfilter service freebsd_committer freebsd_triage 2011-06-27 13:43:02 UTC
Author: ae
Date: Mon Jun 27 12:42:48 2011
New Revision: 223594
URL: http://svn.freebsd.org/changeset/base/223594

Log:
  EBR could contain an early stage of boot code. But we do not support it.
  Remove message about non empty bootcode, we can not break something
  while GEOM_PART_EBR_COMPAT is defined.
  
  But without GEOM_PART_EBR_COMPAT any changes in EBR are allowed and we
  can accidentally wipe the boot code. To do not break anything save
  the first EBR chunk and keep it untouched each time when we are
  changing EBR. Note that we are still not support boot code for EBR.
  
  PR:		kern/141235
  MFC after:	1 month

Modified:
  head/sys/geom/part/g_part_ebr.c

Modified: head/sys/geom/part/g_part_ebr.c
==============================================================================
--- head/sys/geom/part/g_part_ebr.c	Mon Jun 27 12:21:11 2011	(r223593)
+++ head/sys/geom/part/g_part_ebr.c	Mon Jun 27 12:42:48 2011	(r223594)
@@ -59,6 +59,9 @@ FEATURE(geom_part_ebr_compat,
 
 struct g_part_ebr_table {
 	struct g_part_table	base;
+#ifndef GEOM_PART_EBR_COMPAT
+	u_char		ebr[EBRSIZE];
+#endif
 };
 
 struct g_part_ebr_entry {
@@ -459,7 +462,7 @@ g_part_ebr_read(struct g_part_table *bas
 	u_char *buf;
 	off_t ofs, msize;
 	u_int lba;
-	int error, index, sum;
+	int error, index;
 
 	pp = cp->provider;
 	table = (struct g_part_ebr_table *)basetable;
@@ -482,20 +485,11 @@ g_part_ebr_read(struct g_part_table *bas
 			printf("GEOM: %s: invalid entries in the EBR ignored.\n",
 			    pp->name);
 		}
-		/* We do not support bootcode for EBR. If bootcode area is
-		 * not zeroes, then mark this EBR as corrupt to do not break
-		 * anything for another OS'es.
-		 */
-		if (lba == 0) {
-			sum = 0;
-			for (index = 0; index < DOSPARTOFF; index++)
-				sum += buf[index];
-			if (sum != 0) {
-				basetable->gpt_corrupt = 1;
-				printf("GEOM: %s: EBR has non empty bootcode.\n",
-				    pp->name);
-			}
-		}
+#ifndef GEOM_PART_EBR_COMPAT
+		/* Save the first EBR, it can contain a boot code */
+		if (lba == 0)
+			bcopy(buf, table->ebr, sizeof(table->ebr));
+#endif
 		g_free(buf);
 
 		if (ent[0].dp_typ == 0)
@@ -583,6 +577,9 @@ g_part_ebr_type(struct g_part_table *bas
 static int
 g_part_ebr_write(struct g_part_table *basetable, struct g_consumer *cp)
 {
+#ifndef GEOM_PART_EBR_COMPAT
+	struct g_part_ebr_table *table;
+#endif
 	struct g_provider *pp;
 	struct g_part_entry *baseentry, *next;
 	struct g_part_ebr_entry *entry;
@@ -592,6 +589,10 @@ g_part_ebr_write(struct g_part_table *ba
 
 	pp = cp->provider;
 	buf = g_malloc(pp->sectorsize, M_WAITOK | M_ZERO);
+#ifndef GEOM_PART_EBR_COMPAT
+	table = (struct g_part_ebr_table *)basetable;
+	bcopy(table->ebr, buf, DOSPARTOFF);
+#endif
 	le16enc(buf + DOSMAGICOFFSET, DOSMAGIC);
 
 	baseentry = LIST_FIRST(&basetable->gpt_entry);
@@ -644,7 +645,10 @@ g_part_ebr_write(struct g_part_table *ba
 
 		error = g_write_data(cp, baseentry->gpe_start * pp->sectorsize,
 		    buf, pp->sectorsize);
-
+#ifndef GEOM_PART_EBR_COMPAT
+		if (baseentry->gpe_start == 0)
+			bzero(buf, DOSPARTOFF);
+#endif
 		baseentry = next;
 	} while (!error && baseentry != NULL);
 
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org"
Comment 28 Andrey V. Elsukov freebsd_committer freebsd_triage 2011-07-27 05:23:37 UTC
State Changed
From-To: patched->closed

Merged to stable/8. Thanks!
Comment 29 dfilter service freebsd_committer freebsd_triage 2011-07-27 05:23:41 UTC
Author: ae
Date: Wed Jul 27 04:23:26 2011
New Revision: 224465
URL: http://svn.freebsd.org/changeset/base/224465

Log:
  MFC r216754:
    Make EBR probe method less strictly to be able detect EBRs with
    small non fatal inconsistency. EBR may contain boot loader and sometimes
    it just has some garbage data. Now this does not prevent FreeBSD to use
    extended partitions. But since we do not support bootcode for EBR we mark
    tables which have non empty boot area as corrupt. This does make them
    readonly and we can not damage this data.
  
  MFC r223594:
    EBR could contain an early stage of boot code. But we do not support it.
    Remove message about non empty bootcode, we can not break something
    while GEOM_PART_EBR_COMPAT is defined.
  
    But without GEOM_PART_EBR_COMPAT any changes in EBR are allowed and we
    can accidentally wipe the boot code. To do not break anything save
    the first EBR chunk and keep it untouched each time when we are
    changing EBR. Note that we are still not support boot code for EBR.
  
    PR:		kern/141235

Modified:
  stable/8/sys/geom/part/g_part_ebr.c
Directory Properties:
  stable/8/sys/   (props changed)
  stable/8/sys/amd64/include/xen/   (props changed)
  stable/8/sys/cddl/contrib/opensolaris/   (props changed)
  stable/8/sys/contrib/dev/acpica/   (props changed)
  stable/8/sys/contrib/pf/   (props changed)
  stable/8/sys/geom/label/   (props changed)

Modified: stable/8/sys/geom/part/g_part_ebr.c
==============================================================================
--- stable/8/sys/geom/part/g_part_ebr.c	Wed Jul 27 04:10:32 2011	(r224464)
+++ stable/8/sys/geom/part/g_part_ebr.c	Wed Jul 27 04:23:26 2011	(r224465)
@@ -51,6 +51,9 @@ __FBSDID("$FreeBSD$");
 
 struct g_part_ebr_table {
 	struct g_part_table	base;
+#ifndef GEOM_PART_EBR_COMPAT
+	u_char		ebr[EBRSIZE];
+#endif
 };
 
 struct g_part_ebr_entry {
@@ -395,7 +398,7 @@ g_part_ebr_probe(struct g_part_table *ta
 	char psn[8];
 	struct g_provider *pp;
 	u_char *buf, *p;
-	int error, index, res, sum;
+	int error, index, res;
 	uint16_t magic;
 
 	pp = cp->provider;
@@ -427,29 +430,11 @@ g_part_ebr_probe(struct g_part_table *ta
 	if (magic != DOSMAGIC)
 		goto out;
 
-	/*
-	 * The sector is all zeroes, except for the partition entries,
-	 * pseudo boot code and some signatures or disk serial number.
-	 * The latter can be found in the 9 bytes immediately in front
-	 * of the partition table.
-	 */
-	sum = 0;
-	for (index = 96; index < DOSPARTOFF - 9; index++)
-		sum += buf[index];
-	if (sum != 0)
-		goto out;
-
-	for (index = 0; index < NDOSPART; index++) {
+	for (index = 0; index < 2; index++) {
 		p = buf + DOSPARTOFF + index * DOSPARTSIZE;
 		if (p[0] != 0 && p[0] != 0x80)
 			goto out;
-		if (index < 2)
-			continue;
-		/* The 3rd & 4th entries are always zero. */
-		if ((le64dec(p+0) + le64dec(p+8)) != 0)
-			goto out;
 	}
-
 	res = G_PART_PROBE_PRI_NORM;
 
  out:
@@ -483,6 +468,19 @@ g_part_ebr_read(struct g_part_table *bas
 
 		ebr_entry_decode(buf + DOSPARTOFF + 0 * DOSPARTSIZE, ent + 0);
 		ebr_entry_decode(buf + DOSPARTOFF + 1 * DOSPARTSIZE, ent + 1);
+
+		/* The 3rd & 4th entries should be zeroes. */
+		if (le64dec(buf + DOSPARTOFF + 2 * DOSPARTSIZE) +
+		    le64dec(buf + DOSPARTOFF + 3 * DOSPARTSIZE) != 0) {
+			basetable->gpt_corrupt = 1;
+			printf("GEOM: %s: invalid entries in the EBR ignored.\n",
+			    pp->name);
+		}
+#ifndef GEOM_PART_EBR_COMPAT
+		/* Save the first EBR, it can contain a boot code */
+		if (lba == 0)
+			bcopy(buf, table->ebr, sizeof(table->ebr));
+#endif
 		g_free(buf);
 
 		if (ent[0].dp_typ == 0)
@@ -570,6 +568,9 @@ g_part_ebr_type(struct g_part_table *bas
 static int
 g_part_ebr_write(struct g_part_table *basetable, struct g_consumer *cp)
 {
+#ifndef GEOM_PART_EBR_COMPAT
+	struct g_part_ebr_table *table;
+#endif
 	struct g_provider *pp;
 	struct g_part_entry *baseentry, *next;
 	struct g_part_ebr_entry *entry;
@@ -579,6 +580,10 @@ g_part_ebr_write(struct g_part_table *ba
 
 	pp = cp->provider;
 	buf = g_malloc(pp->sectorsize, M_WAITOK | M_ZERO);
+#ifndef GEOM_PART_EBR_COMPAT
+	table = (struct g_part_ebr_table *)basetable;
+	bcopy(table->ebr, buf, DOSPARTOFF);
+#endif
 	le16enc(buf + DOSMAGICOFFSET, DOSMAGIC);
 
 	baseentry = LIST_FIRST(&basetable->gpt_entry);
@@ -631,7 +636,10 @@ g_part_ebr_write(struct g_part_table *ba
 
 		error = g_write_data(cp, baseentry->gpe_start * pp->sectorsize,
 		    buf, pp->sectorsize);
-
+#ifndef GEOM_PART_EBR_COMPAT
+		if (baseentry->gpe_start == 0)
+			bzero(buf, DOSPARTOFF);
+#endif
 		baseentry = next;
 	} while (!error && baseentry != NULL);
 
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org"