[PATCH v3 1/1] partitions/msdos: FreeBSD UFS2 file systems are not recognized

[PATCH v3 1/1] partitions/msdos: FreeBSD UFS2 file systems are not recognized

[Richard Narron]

The code in block/partitions/msdos.c recognizes FreeBSD, OpenBSD
and NetBSD partitions and does a reasonable job picking out OpenBSD
and NetBSD UFS subpartitions.

But for FreeBSD the subpartitions are always "bad".

     Kernel: <bsd:bad subpartition - ignored

Though all 3 of these BSD systems use UFS as a file system, only
FreeBSD uses relative start addresses in the subpartition
declarations.

The following patch fixes this for FreeBSD partitions and leaves
the code for OpenBSD and NetBSD intact:

Cc: Jens Axboe <axboe@kernel.dk>
Cc: Andries Brouwer <aeb@cwi.nl>
Cc: linux-block@vger.kernel.org
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Richard Narron <comet.berkeley@gmail.com>
---
Changelog v2->v3:
- Add Cc:
Changelog v1->v2: - Improve style, use +=
---
  block/partitions/msdos.c | 2 ++
  1 file changed, 2 insertions(+)

--- a/block/partitions/msdos.c	2015-12-27 18:17:37.000000000 -0800
+++ b/block/partitions/msdos.c	2015-12-29 10:44:25.813773357 -0800
@@ -300,6 +300,8 @@ static void parse_bsd(struct parsed_part
  			continue;
  		bsd_start = le32_to_cpu(p->p_offset);
  		bsd_size = le32_to_cpu(p->p_size);
+		if (memcmp(flavour, "bsd\0", 4) == 0)
+			bsd_start += offset;
  		if (offset == bsd_start && size == bsd_size)
  			/* full parent partition, we have it already */
  			continue;

Re: [PATCH v3 1/1] partitions/msdos: FreeBSD UFS2 file systems are not recognized

[Jens Axboe]

On 05/26/2017 04:48 AM, Richard Narron wrote:
> The code in block/partitions/msdos.c recognizes FreeBSD, OpenBSD
> and NetBSD partitions and does a reasonable job picking out OpenBSD
> and NetBSD UFS subpartitions.
> 
> But for FreeBSD the subpartitions are always "bad".
> 
>      Kernel: <bsd:bad subpartition - ignored
> 
> Though all 3 of these BSD systems use UFS as a file system, only
> FreeBSD uses relative start addresses in the subpartition
> declarations.
> 
> The following patch fixes this for FreeBSD partitions and leaves
> the code for OpenBSD and NetBSD intact:

I queued this up 3 days ago, and replied as such. There's no need
to keep sending it.

-- 
Jens Axboe

Re: [PATCH v3 1/1] partitions/msdos: FreeBSD UFS2 file systems are not recognized

[Joe Perches]

On Fri, 2017-05-26 at 03:48 -0700, Richard Narron wrote:
> The code in block/partitions/msdos.c recognizes FreeBSD, OpenBSD
> and NetBSD partitions and does a reasonable job picking out OpenBSD
> and NetBSD UFS subpartitions.
> 
> But for FreeBSD the subpartitions are always "bad".
> 
>      Kernel: <bsd:bad subpartition - ignored
[]
>   block/partitions/msdos.c | 2 ++
[]
> @@ -300,6 +300,8 @@ static void parse_bsd(struct parsed_part
>   			continue;
>   		bsd_start = le32_to_cpu(p->p_offset);
>   		bsd_size = le32_to_cpu(p->p_size);
> +		if (memcmp(flavour, "bsd\0", 4) == 0)

Weird code.  Why not:

		if (strcmp(flavor, "bsd") == 0)

Re: [PATCH v3 1/1] partitions/msdos: FreeBSD UFS2 file systems are not recognized

[Richard Narron]

On Fri, 26 May 2017, Joe Perches wrote:

> On Fri, 2017-05-26 at 03:48 -0700, Richard Narron wrote:
>> The code in block/partitions/msdos.c recognizes FreeBSD, OpenBSD
>> and NetBSD partitions and does a reasonable job picking out OpenBSD
>> and NetBSD UFS subpartitions.
>>
>> But for FreeBSD the subpartitions are always "bad".
>>
>>      Kernel: <bsd:bad subpartition - ignored
> []
>>   block/partitions/msdos.c | 2 ++
> []
>> @@ -300,6 +300,8 @@ static void parse_bsd(struct parsed_part
>>   			continue;
>>   		bsd_start = le32_to_cpu(p->p_offset);
>>   		bsd_size = le32_to_cpu(p->p_size);
>> +		if (memcmp(flavour, "bsd\0", 4) == 0)
>
> Weird code.  Why not:
>
> 		if (strcmp(flavor, "bsd") == 0)
>

I instinctively trust the memcmp function as it seems more like 
assembly language to me and more straight forward and more reliable than 
strcmp.

I'm new to this forum and did not know who you were so I looked up your 
name in old threads and oddly enough found this thread about a strcmp bug:

http://marc.info/?l=linux-kernel&m=125848558316468&w=2

http://marc.info/?l=linux-kernel&m=125847802903422&w=2

Re: [PATCH v3 1/1] partitions/msdos: FreeBSD UFS2 file systems are not recognized

[Joe Perches]

On Fri, 2017-05-26 at 16:30 -0700, Richard Narron wrote:
> On Fri, 26 May 2017, Joe Perches wrote:
> 
> > On Fri, 2017-05-26 at 03:48 -0700, Richard Narron wrote:
> > > The code in block/partitions/msdos.c recognizes FreeBSD, OpenBSD
> > > and NetBSD partitions and does a reasonable job picking out OpenBSD
> > > and NetBSD UFS subpartitions.
> > > 
> > > But for FreeBSD the subpartitions are always "bad".
> > > 
> > >      Kernel: <bsd:bad subpartition - ignored
> > 
> > []
> > >   block/partitions/msdos.c | 2 ++
> > 
> > []
> > > @@ -300,6 +300,8 @@ static void parse_bsd(struct parsed_part
> > >   			continue;
> > >   		bsd_start = le32_to_cpu(p->p_offset);
> > >   		bsd_size = le32_to_cpu(p->p_size);
> > > +		if (memcmp(flavour, "bsd\0", 4) == 0)
> > 
> > Weird code.  Why not:
> > 
> > 		if (strcmp(flavor, "bsd") == 0)
> > 
> 
> I instinctively trust the memcmp function as it seems more like 
> assembly language to me and more straight forward and more reliable than 
> strcmp.

That really doesn't matter.

Your code stores "bsd\0\0" and not just "bsd\0"

Re: [PATCH v3 1/1] partitions/msdos: FreeBSD UFS2 file systems are not recognized

[Joe Perches]

(please keep replies on the list)

On Fri, 2017-05-26 at 18:33 -0700, Richard Narron wrote:
> On Fri, 26 May 2017, Joe Perches wrote:
> > On Fri, 2017-05-26 at 16:30 -0700, Richard Narron wrote:
> > > On Fri, 26 May 2017, Joe Perches wrote:
> > > > On Fri, 2017-05-26 at 03:48 -0700, Richard Narron wrote:
> > > > > The code in block/partitions/msdos.c recognizes FreeBSD, OpenBSD
> > > > > and NetBSD partitions and does a reasonable job picking out OpenBSD
> > > > > and NetBSD UFS subpartitions.
> > > > > 
> > > > > But for FreeBSD the subpartitions are always "bad".
> > > > > 
> > > > >      Kernel: <bsd:bad subpartition - ignored
> > > > 
> > > > []
> > > > >   block/partitions/msdos.c | 2 ++
> > > > 
> > > > []
> > > > > @@ -300,6 +300,8 @@ static void parse_bsd(struct parsed_part
> > > > >   			continue;
> > > > >   		bsd_start = le32_to_cpu(p->p_offset);
> > > > >   		bsd_size = le32_to_cpu(p->p_size);
> > > > > +		if (memcmp(flavour, "bsd\0", 4) == 0)
> > > > 
> > > > Weird code.  Why not:
> > > > 
> > > > 		if (strcmp(flavor, "bsd") == 0)
> > > > 
> > > 
> > > I instinctively trust the memcmp function as it seems more like
> > > assembly language to me and more straight forward and more reliable than
> > > strcmp.
> > 
> > That really doesn't matter.
> > 
> > Your code stores "bsd\0\0" and not just "bsd\0"
> > 
> 
> Thanks for looking at this code. I do appreciate it.
> 
> How about saving a byte and doing this instead?
> 
>            if (memcmp(flavour, "bsd", 4) == 0)
> 
> I do appreciate your input as coding style is important, but so too is 
> reliability.
> 
> I don't trust the string functions and probably never will.
> 
> It is not surprising to me that things like SQL injection and any number of other 
> C string exploits are very common.
> 
> IBM gave up on the idea of marking memory to keep track of data length with the 1401 machines in the 1950's.
> 
> But Digital Equipment kept the idea alive of using null characters for a 
> long time.  Sadly the C programming language copied this bad idea for 
> strings.

Let's not argue the language.

Please use what's normal for the language as that is
readers of the code typically expect.

Re: [PATCH v3 1/1] partitions/msdos: FreeBSD UFS2 file systems are not recognized

[Richard Narron]

On Fri, 26 May 2017, Joe Perches wrote:

> (please keep replies on the list)
>
> On Fri, 2017-05-26 at 18:33 -0700, Richard Narron wrote:
>> On Fri, 26 May 2017, Joe Perches wrote:
>>> On Fri, 2017-05-26 at 16:30 -0700, Richard Narron wrote:
>>>> On Fri, 26 May 2017, Joe Perches wrote:
>>>>> On Fri, 2017-05-26 at 03:48 -0700, Richard Narron wrote:
>>>>>> The code in block/partitions/msdos.c recognizes FreeBSD, OpenBSD
>>>>>> and NetBSD partitions and does a reasonable job picking out OpenBSD
>>>>>> and NetBSD UFS subpartitions.
>>>>>>
>>>>>> But for FreeBSD the subpartitions are always "bad".
>>>>>>
>>>>>>      Kernel: <bsd:bad subpartition - ignored
>>>>>
>>>>> []
>>>>>>   block/partitions/msdos.c | 2 ++
>>>>>
>>>>> []
>>>>>> @@ -300,6 +300,8 @@ static void parse_bsd(struct parsed_part
>>>>>>   			continue;
>>>>>>   		bsd_start = le32_to_cpu(p->p_offset);
>>>>>>   		bsd_size = le32_to_cpu(p->p_size);
>>>>>> +		if (memcmp(flavour, "bsd\0", 4) == 0)
>>>>>
>>>>> Weird code.  Why not:
>>>>>
>>>>> 		if (strcmp(flavor, "bsd") == 0)
>>>>>
>>>>
>>>> I instinctively trust the memcmp function as it seems more like
>>>> assembly language to me and more straight forward and more reliable than
>>>> strcmp.
>>>
>>> That really doesn't matter.
>>>
>>> Your code stores "bsd\0\0" and not just "bsd\0"
>>>
>>
>> Thanks for looking at this code. I do appreciate it.
>>
>> How about saving a byte and doing this instead?
>>
>>            if (memcmp(flavour, "bsd", 4) == 0)
>>
>> I do appreciate your input as coding style is important, but so too is
>> reliability.
>>
>> I don't trust the string functions and probably never will.
>>
>> It is not surprising to me that things like SQL injection and any number of other
>> C string exploits are very common.
>>
>> IBM gave up on the idea of marking memory to keep track of data length with the 1401 machines in the 1950's.
>>
>> But Digital Equipment kept the idea alive of using null characters for a
>> long time.  Sadly the C programming language copied this bad idea for
>> strings.
>
> Let's not argue the language.
>
> Please use what's normal for the language as that is
> readers of the code typically expect.
>

Under the /block/partitions directory the c programs have about 13 uses 
of memcmp() and 6 uses of strcmp().

Re: [PATCH v3 1/1] partitions/msdos: FreeBSD UFS2 file systems are not recognized

[Joe Perches]

On Fri, 2017-05-26 at 20:20 -0700, Richard Narron wrote:
> Under the /block/partitions directory the c programs have about 13 uses 
> of memcmp() and 6 uses of strcmp().

Nearly all of the memcmp uses with strings kernel wide use
the equivalent of memcmp(foo, "bar", strlen("bar"));

[PATCH v3 1/1] partitions/msdos: FreeBSD UFS2 file systems are not recognized

[Richard Narron]

The code in block/partitions/msdos.c recognizes FreeBSD, OpenBSD
and NetBSD partitions and does a reasonable job picking out OpenBSD
and NetBSD UFS subpartitions.

But for FreeBSD the subpartitions are always "bad".

     Kernel: <bsd:bad subpartition - ignored

Though all 3 of these BSD systems use UFS as a file system, only
FreeBSD uses relative start addresses in the subpartition
declarations.

The following patch fixes this for FreeBSD partitions and leaves
the code for OpenBSD and NetBSD intact:

Cc: Jens Axboe <axboe@kernel.dk>
Cc: Andries Brouwer <aeb@cwi.nl>
Cc: linux-block@vger.kernel.org
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Richard Narron <comet.berkeley@gmail.com>
---
Changelog v2->v3:
- Add Cc:
Changelog v1->v2: 
- Improve style, use += 
---
  block/partitions/msdos.c | 2 ++
  1 file changed, 2 insertions(+)

--- a/block/partitions/msdos.c	2015-12-27 18:17:37.000000000 -0800
+++ b/block/partitions/msdos.c	2015-12-29 10:44:25.813773357 -0800
@@ -300,6 +300,8 @@ static void parse_bsd(struct parsed_part
  			continue;
  		bsd_start = le32_to_cpu(p->p_offset);
  		bsd_size = le32_to_cpu(p->p_size);
+		if (memcmp(flavour, "bsd\0", 4) == 0)
+			bsd_start += offset;
  		if (offset == bsd_start && size == bsd_size)
  			/* full parent partition, we have it already */
  			continue;