Re: [PATCH] fs/befs/linuxvfs.c: need signed cast for variable 'block'

Chen Gang
2013-11-02T13:52:36Z

On 11/01/2013 10:41 AM, Chen Gang wrote:
> On 11/01/2013 04:45 AM, Greg KH wrote:
>> On Thu, Oct 31, 2013 at 12:08:33PM -0700, Kees Cook wrote:
>>> On Thu, Oct 31, 2013 at 12:06 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>>>> On Thu, Oct 31, 2013 at 09:53:59AM -0700, Kees Cook wrote:
>>>>
>>>>> If block (type sector_t) is unsigned, we shouldn't cast it signed.
>>>>> This entire code path should be removed. What is BEFS's expected
>>>>> maximum block size? (Looks like even befs_blocknr_t is u64, so nothing
>>>>> seems trivially in danger of wrapping.) I would also note that all the
>>>>> format strings are wrong too (%ld instead of %lu).
>>>>
>>>> FWIW, this
>>>> res = befs_fblock2brun(sb, ds, block, &run);
>>>> if (res != BEFS_OK) {
>>>> befs_error(sb,
>>>> "<--- befs_get_block() for inode %lu, block "
>>>> "%ld ERROR", inode->i_ino, block);
>>>> return -EFBIG;
>>>> }
>>>> also looks wrong - ioctl(..., FIBMAP, ...) shouldn't be able to spew
>>>> printks on a valid fs and hitting it with block number greater than
>>>> file length will, AFAICS, trigger that.
>>>>
>>>> I agree that this code needs fixing, but just making gcc STFU about the
>>>> comparison would only serve to hide the problem. Anybody familiar with
>>>> befs or willing to learn it?
>>>
>>> Agreed. MAINTAINERS shows it as orphaned. Perhaps it should be moved
>>> into staging?
>>
>> Only if we want to delete the thing. I'll be glad to take it there, and
>> remove it in 2 releases and then if anyone complains, we can add it back
>> easily. Just let me know.
>>
>
> Excuse me, I am not quite familiar with BEFS, I guess your meaning is:
>
> "if it is no further more discussion (e.g. within 1 week, no members reply), you will remove it (take it to "drivers/staging" sub-directory)".
>

Oh, for me, it is not suitable to move a file system sub-directory to
"drivers/*/" sub-directory. And I can not find any sub-directory like
'staging' under "fs" sub-directory, either.

Do we have any sub-directory like "staging" in "fs" sub-directory? if
no, do we have to create it or have to use another ways instead of?

After reference the "drivers/staging" explanation (list below), we know
it is only for drivers (not fs).

menuconfig STAGING
bool "Staging drivers"
default n
---help---
This option allows you to select a number of drivers that are
not of the "normal" Linux kernel quality level. These drivers
are placed here in order to get a wider audience to make use of
them. Please note that these drivers are under heavy
development, may or may not work, and may contain userspace
interfaces that most likely will be changed in the near
future.

Using any of these drivers will taint your kernel which might
affect support options from both the community, and various
commercial support organizations.

If you wish to work on these drivers, to help improve them, or
to report problems you have with them, please see the
driver_name.README file in the drivers/staging/ directory to
see what needs to be worked on, and who to contact.

If in doubt, say N here.

Thanks.

> If what I guess is correct, I support you (else, please let me know)
>
>
> Thanks.
>

--
Chen Gang