| lists.openwall.net | 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 linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <6b22baa6-a75f-45ff-a0cb-33ae0ecf4720@126.com>
Date: Tue, 24 Sep 2024 20:08:56 +0800
From: Zhao Mengmeng <zhaomzhao@....com>
To: Jan Kara <jack@...e.cz>
Cc: jack@...e.com, zhaomengmeng@...inos.cn, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/3] udf: refactor udf_next_aext() to handle error
On 2024/9/20 23:47, Jan Kara wrote:
> On Wed 18-09-24 17:36:33, Zhao Mengmeng wrote:
>> From: Zhao Mengmeng <zhaomengmeng@...inos.cn>
>>
>> Same as udf_current_aext(), take pointer to etype to store the extent
>> type, while return 0 for success and <0 on error.
>>
>> Signed-off-by: Zhao Mengmeng <zhaomengmeng@...inos.cn>
>
> ...
>
>> diff --git a/fs/udf/directory.c b/fs/udf/directory.c
>> index 93153665eb37..f865538c985d 100644
>> --- a/fs/udf/directory.c
>> +++ b/fs/udf/directory.c
>> @@ -166,13 +166,16 @@ static struct buffer_head *udf_fiiter_bread_blk(struct udf_fileident_iter *iter)
>> */
>> static int udf_fiiter_advance_blk(struct udf_fileident_iter *iter)
>> {
>> + int8_t etype;
>> + int err = 0;
>
> Nit: please add empty line between declaration and the code.
>
>> iter->loffset++;
>> if (iter->loffset < DIV_ROUND_UP(iter->elen, 1<<iter->dir->i_blkbits))
>> return 0;
>>
>> iter->loffset = 0;
>> - if (udf_next_aext(iter->dir, &iter->epos, &iter->eloc, &iter->elen, 1)
>> - != (EXT_RECORDED_ALLOCATED >> 30)) {
>> + err = udf_next_aext(iter->dir, &iter->epos, &iter->eloc, &iter->elen,
>> + &etype, 1);
>> + if (err || etype != (EXT_RECORDED_ALLOCATED >> 30)) {
>> if (iter->pos == iter->dir->i_size) {
>> iter->elen = 0;
>> return 0;
>
> ...
>
>> @@ -555,7 +556,7 @@ static int udf_do_extend_file(struct inode *inode,
>> * empty indirect extent.
>> */
>> if (new_block_bytes)
>> - udf_next_aext(inode, last_pos, &tmploc, &tmplen, 0);
>> + udf_next_aext(inode, last_pos, &tmploc, &tmplen, &tmptype, 0);
>> }
>> iinfo->i_lenExtents += add;
>>
>
> Hum, this will need error checking but we can leave that for future
> patches.
>
>> @@ -674,8 +675,8 @@ static int udf_extend_file(struct inode *inode, loff_t newsize)
>> extent.extLength = EXT_NOT_RECORDED_NOT_ALLOCATED;
>> } else {
>> epos.offset -= adsize;
>> - etype = udf_next_aext(inode, &epos, &extent.extLocation,
>> - &extent.extLength, 0);
>> + udf_next_aext(inode, &epos, &extent.extLocation,
>> + &extent.extLength, &etype, 0);
>> extent.extLength |= etype << 30;
>> }
>>
>> @@ -712,7 +713,7 @@ static int inode_getblk(struct inode *inode, struct udf_map_rq *map)
>> loff_t lbcount = 0, b_off = 0;
>> udf_pblk_t newblocknum;
>> sector_t offset = 0;
>> - int8_t etype;
>> + int8_t etype, tmpetype;
>> struct udf_inode_info *iinfo = UDF_I(inode);
>> udf_pblk_t goal = 0, pgoal = iinfo->i_location.logicalBlockNum;
>> int lastblock = 0;
>> @@ -748,8 +749,8 @@ static int inode_getblk(struct inode *inode, struct udf_map_rq *map)
>> prev_epos.offset = cur_epos.offset;
>> cur_epos.offset = next_epos.offset;
>>
>> - etype = udf_next_aext(inode, &next_epos, &eloc, &elen, 1);
>> - if (etype == -1)
>> + ret = udf_next_aext(inode, &next_epos, &eloc, &elen, &etype, 1);
>> + if (ret)
>> break;
>
> I think here we need to add error handling as well and we should probably
> do it in this patch / patch series. If ret is ENODATA, we just break out
> from the cycle but if ret is some other error, we need to return that error
> from inode_getblk().
>
>> @@ -771,8 +772,8 @@ static int inode_getblk(struct inode *inode, struct udf_map_rq *map)
>> * Move prev_epos and cur_epos into indirect extent if we are at
>> * the pointer to it
>> */
>> - udf_next_aext(inode, &prev_epos, &tmpeloc, &tmpelen, 0);
>> - udf_next_aext(inode, &cur_epos, &tmpeloc, &tmpelen, 0);
>> + udf_next_aext(inode, &prev_epos, &tmpeloc, &tmpelen, &tmpetype, 0);
>> + udf_next_aext(inode, &cur_epos, &tmpeloc, &tmpelen, &tmpetype, 0);
>
> Again, this should have error handling now.
>
>>
>> /* if the extent is allocated and recorded, return the block
>> if the extent is not a multiple of the blocksize, round up */
>> @@ -793,7 +794,7 @@ static int inode_getblk(struct inode *inode, struct udf_map_rq *map)
>> }
>>
>> /* Are we beyond EOF and preallocated extent? */
>> - if (etype == -1) {
>> + if (ret < 0) {
>
> I'd prefer ret == -ENODATA to make this explicit.
>
>> loff_t hole_len;
>>
>> isBeyondEOF = true;
>> @@ -846,8 +847,7 @@ static int inode_getblk(struct inode *inode, struct udf_map_rq *map)
>>
>> /* if the current block is located in an extent,
>> read the next extent */
>> - etype = udf_next_aext(inode, &next_epos, &eloc, &elen, 0);
>> - if (etype != -1) {
>> + if (!udf_next_aext(inode, &next_epos, &eloc, &elen, &etype, 0)) {
>> laarr[c + 1].extLength = (etype << 30) | elen;
>> laarr[c + 1].extLocation = eloc;
>> count++;
>
> And this should be distinguisting between EOF and other errors so that we
> don't set lastblock wrongly. Instead we should bail with error.
>
>> @@ -1190,13 +1191,13 @@ static int udf_update_extents(struct inode *inode, struct kernel_long_ad *laarr,
>> if (err < 0)
>> return err;
>> udf_next_aext(inode, epos, &laarr[i].extLocation,
>> - &laarr[i].extLength, 1);
>> + &laarr[i].extLength, &tmptype, 1);
>> start++;
>> }
>> }
>>
>> for (i = start; i < endnum; i++) {
>> - udf_next_aext(inode, epos, &tmploc, &tmplen, 0);
>> + udf_next_aext(inode, epos, &tmploc, &tmplen, &tmptype, 0);
>> udf_write_aext(inode, epos, &laarr[i].extLocation,
>> laarr[i].extLength, 1);
>> }
>
> Again these two calls should have error handling now. udf_update_extents()
> is already able to return errors.
>
>> @@ -2267,7 +2268,7 @@ static int udf_insert_aext(struct inode *inode, struct extent_position epos,
>> if (epos.bh)
>> get_bh(epos.bh);
>>
>> - while ((etype = udf_next_aext(inode, &epos, &oeloc, &oelen, 0)) != -1) {
>> + while (!udf_next_aext(inode, &epos, &oeloc, &oelen, &etype, 0)) {
>> udf_write_aext(inode, &epos, &neloc, nelen, 1);
>> neloc = oeloc;
>> nelen = (etype << 30) | oelen;
>
> Here, we should check if udf_next_aext() returned error (other than
> ENODATA) and bail in that case instead of trying to insert new extent.
>
>> @@ -2302,10 +2303,10 @@ int8_t udf_delete_aext(struct inode *inode, struct extent_position epos)
>> adsize = 0;
>>
>> oepos = epos;
>> - if (udf_next_aext(inode, &epos, &eloc, &elen, 1) == -1)
>> + if (udf_next_aext(inode, &epos, &eloc, &elen, &etype, 1))
>> return -1;
>>
>> - while ((etype = udf_next_aext(inode, &epos, &eloc, &elen, 1)) != -1) {
>> + while (!udf_next_aext(inode, &epos, &eloc, &elen, &etype, 1)) {
>> udf_write_aext(inode, &oepos, &eloc, (etype << 30) | elen, 1);
>> if (oepos.bh != epos.bh) {
>> oepos.block = epos.block;
>> @@ -2379,8 +2380,7 @@ int8_t inode_bmap(struct inode *inode, sector_t block,
>> }
>> *elen = 0;
>> do {
>> - etype = udf_next_aext(inode, pos, eloc, elen, 1);
>> - if (etype == -1) {
>> + if (udf_next_aext(inode, pos, eloc, elen, &etype, 1)) {
>> *offset = (bcount - lbcount) >> blocksize_bits;
>> iinfo->i_lenExtents = lbcount;
>> return -1;
>
> Again, here we need to distinguish ENODATA from other errors so that we
> don't wrongly consider failure to read extent like EOF.
>
>> diff --git a/fs/udf/truncate.c b/fs/udf/truncate.c
>> index 91b6e2698e7e..b7361222f988 100644
>> --- a/fs/udf/truncate.c
>> +++ b/fs/udf/truncate.c
>> @@ -85,7 +85,7 @@ void udf_truncate_tail_extent(struct inode *inode)
>> BUG();
>>
>> /* Find the last extent in the file */
>> - while ((netype = udf_next_aext(inode, &epos, &eloc, &elen, 1)) != -1) {
>> + while (!udf_next_aext(inode, &epos, &eloc, &elen, &netype, 1)) {
>> etype = netype;
>> lbcount += elen;
>> if (lbcount > inode->i_size) {
>
> This should be checking for error (after the loop) so that we don't
> accidentally try to truncate extents early in case of error.
>
Sorry to bother, in case of error(including EOF), it won't go into the loop and
has chance to call extent_trunc(). After the loop, only some update and clean op,
iinfo->i_lenExtents = inode->i_size;
brelse(epos.bh);
So I'm a little confused which part of this piece of code needs to change?
Powered by blists - more mailing lists