lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  PHC 
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Tue, 5 May 2020 12:37:29 +0200
From:   Christoph Hellwig <>
To:     Ritesh Harjani <>
Cc:     Christoph Hellwig <>,,,,,,,,
Subject: Re: [PATCH 08/11] fs: move fiemap range validation into the file
 systems instances

On Sat, May 02, 2020 at 04:19:05AM +0530, Ritesh Harjani wrote:
>> -int fiemap_check_flags(struct fiemap_extent_info *fieinfo, u32 fs_flags);
>> +fiemap_prep() helper.
> Let's add the fiemap_prep() function prototype here.
> So that below description would make more sense.

The annoying tendency to copy prototypes into docs just means they
get out of date.  Anyone who can't do a quick grep should not be
writing code.  I've added it back for consistency, but we really
should remove all of them in this file.

>> ioctl_fiemap(). The
>>   set of fiemap flags which the fs understands should be passed via fs_flags. If
>> -fiemap_check_flags finds invalid user flags, it will place the bad values in
>> +fiemap_prep finds invalid user flags, it will place the bad values in
>>   fieinfo->fi_flags and return -EBADR. If the file system gets -EBADR, from
>> -fiemap_check_flags(), it should immediately exit, returning that error back to
>> +fiemap_prep(), it should immediately exit, returning that error back to
>>   ioctl_fiemap().
> Also maybe we should also add more info about fiemap_prep() helper.
> Since it also checks for invalid len and invalid start range and hence
> could return -EINVAL or -EFBIG.

I've added it.  But this kind of documentation that just badly spells
out what is done is not very useful.  A quick look at the function
conveys the information much better.

>> --- a/fs/cifs/smb2ops.c
>> +++ b/fs/cifs/smb2ops.c
>> @@ -3408,8 +3408,10 @@ static int smb3_fiemap(struct cifs_tcon *tcon,
>>   	int i, num, rc, flags, last_blob;
>>   	u64 next;
>>   -	if (fiemap_check_flags(fei, FIEMAP_FLAG_SYNC))
>> -		return -EBADR;
>> +	rc = fiemap_prep(cfile->dentry->d_inode, fei, start, &len,
> How about d_inode(cfile->dentry) ?


>> +	if (rc)
>> +		rc;
> missed "return rc" here?


>> + * @start:	Start of the mapped range
>> + * @len:	Length of the mapped range, can be truncated by this function.
>> + * @supported_flags:	Set of fiemap flags that the file system understands
>>    *
>>    * Called from file system ->fiemap callback. This will compute the
>>    * intersection of valid fiemap flags and those that the fs supports. That
> Let's also add more documentation about this new fiemap_prep() helper.
> Earlier comments above this function description only talks about
> incompat flags. We should add more info about check_ranges part as well 
> here.

Seriously, the details are in the f^$^$ing code.  No need to have an
easily out of sync version of the code in the comment.  I'll just
radically shorten this.

Powered by blists - more mailing lists