qnx4_block_map() may return -EIO on funny qnx4 fs image, in this case do
not interpret -EIO as a correct address
Signed-off-by: Tong Zhang <[email protected]>
---
fs/qnx4/inode.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/fs/qnx4/inode.c b/fs/qnx4/inode.c
index e8da1cde87b9..d3a40c5b1a9a 100644
--- a/fs/qnx4/inode.c
+++ b/fs/qnx4/inode.c
@@ -59,6 +59,8 @@ static int qnx4_get_block( struct inode *inode, sector_t iblock, struct buffer_h
QNX4DEBUG((KERN_INFO "qnx4: qnx4_get_block inode=[%ld] iblock=[%ld]\n",inode->i_ino,iblock));
phys = qnx4_block_map( inode, iblock );
+ if (phys == -EIO)
+ return -EIO;
if ( phys ) {
// logical block is before EOF
map_bh(bh, inode->i_sb, phys);
--
2.25.1
On Friday, 2020-10-23 23:16 Tong Zhang wrote:
> qnx4_block_map() may return -EIO on funny qnx4 fs image, in this case do
> not interpret -EIO as a correct address
>
> Signed-off-by: Tong Zhang <[email protected]>
> ---
> fs/qnx4/inode.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/fs/qnx4/inode.c b/fs/qnx4/inode.c
> index e8da1cde87b9..d3a40c5b1a9a 100644
> --- a/fs/qnx4/inode.c
> +++ b/fs/qnx4/inode.c
> @@ -59,6 +59,8 @@ static int qnx4_get_block( struct inode *inode, sector_t iblock, struct buffer_h
> QNX4DEBUG((KERN_INFO "qnx4: qnx4_get_block inode=[%ld] iblock=[%ld]\n",inode->i_ino,iblock));
>
> phys = qnx4_block_map( inode, iblock );
> + if (phys == -EIO)
> + return -EIO;
> if ( phys ) {
> // logical block is before EOF
> map_bh(bh, inode->i_sb, phys);
The fix looks sane to me, but how about the two other callers of
qnx4_block_map(), are they not affected as well?
Cheers
Anders
Thanks Anders!
I'm sending out another patch fixing other callers as suggested.
- Tong
On Mon, Nov 2, 2020 at 4:12 AM Anders Larsen <[email protected]> wrote:
>
> On Friday, 2020-10-23 23:16 Tong Zhang wrote:
> > qnx4_block_map() may return -EIO on funny qnx4 fs image, in this case do
> > not interpret -EIO as a correct address
> >
> > Signed-off-by: Tong Zhang <[email protected]>
> > ---
> > fs/qnx4/inode.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/fs/qnx4/inode.c b/fs/qnx4/inode.c
> > index e8da1cde87b9..d3a40c5b1a9a 100644
> > --- a/fs/qnx4/inode.c
> > +++ b/fs/qnx4/inode.c
> > @@ -59,6 +59,8 @@ static int qnx4_get_block( struct inode *inode, sector_t iblock, struct buffer_h
> > QNX4DEBUG((KERN_INFO "qnx4: qnx4_get_block inode=[%ld] iblock=[%ld]\n",inode->i_ino,iblock));
> >
> > phys = qnx4_block_map( inode, iblock );
> > + if (phys == -EIO)
> > + return -EIO;
> > if ( phys ) {
> > // logical block is before EOF
> > map_bh(bh, inode->i_sb, phys);
>
> The fix looks sane to me, but how about the two other callers of
> qnx4_block_map(), are they not affected as well?
>
> Cheers
> Anders
>
>
qnx4_block_map() may return -EIO on funny qnx4 fs image, in this case do
not interpret -EIO as a correct address
Signed-off-by: Tong Zhang <[email protected]>
---
v2: also check other callers according to Anders Larsen's<[email protected]> comment
fs/qnx4/dir.c | 2 ++
fs/qnx4/inode.c | 2 ++
fs/qnx4/namei.c | 3 +++
3 files changed, 7 insertions(+)
diff --git a/fs/qnx4/dir.c b/fs/qnx4/dir.c
index a6ee23aadd28..11aaf59f0411 100644
--- a/fs/qnx4/dir.c
+++ b/fs/qnx4/dir.c
@@ -31,6 +31,8 @@ static int qnx4_readdir(struct file *file, struct dir_context *ctx)
while (ctx->pos < inode->i_size) {
blknum = qnx4_block_map(inode, ctx->pos >> QNX4_BLOCK_SIZE_BITS);
+ if (blknum == -EIO)
+ return -EIO;
bh = sb_bread(inode->i_sb, blknum);
if (bh == NULL) {
printk(KERN_ERR "qnx4_readdir: bread failed (%ld)\n", blknum);
diff --git a/fs/qnx4/inode.c b/fs/qnx4/inode.c
index e8da1cde87b9..d3a40c5b1a9a 100644
--- a/fs/qnx4/inode.c
+++ b/fs/qnx4/inode.c
@@ -59,6 +59,8 @@ static int qnx4_get_block( struct inode *inode, sector_t iblock, struct buffer_h
QNX4DEBUG((KERN_INFO "qnx4: qnx4_get_block inode=[%ld] iblock=[%ld]\n",inode->i_ino,iblock));
phys = qnx4_block_map( inode, iblock );
+ if (phys == -EIO)
+ return -EIO;
if ( phys ) {
// logical block is before EOF
map_bh(bh, inode->i_sb, phys);
diff --git a/fs/qnx4/namei.c b/fs/qnx4/namei.c
index 8d72221735d7..164e0c07e3ff 100644
--- a/fs/qnx4/namei.c
+++ b/fs/qnx4/namei.c
@@ -66,6 +66,8 @@ static struct buffer_head *qnx4_find_entry(int len, struct inode *dir,
while (blkofs * QNX4_BLOCK_SIZE + offset < dir->i_size) {
if (!bh) {
block = qnx4_block_map(dir, blkofs);
+ if (block == -EIO)
+ goto out;
if (block)
bh = sb_bread(dir->i_sb, block);
if (!bh) {
@@ -88,6 +90,7 @@ static struct buffer_head *qnx4_find_entry(int len, struct inode *dir,
blkofs++;
}
brelse(bh);
+out:
*res_dir = NULL;
return NULL;
}
--
2.25.1
From: Tong Zhang
> Sent: 02 November 2020 20:16
>
> qnx4_block_map() may return -EIO on funny qnx4 fs image, in this case do
> not interpret -EIO as a correct address
'Block number' not 'address'.
> Signed-off-by: Tong Zhang <[email protected]>
> ---
> v2: also check other callers according to Anders Larsen's<[email protected]> comment
> fs/qnx4/dir.c | 2 ++
> fs/qnx4/inode.c | 2 ++
> fs/qnx4/namei.c | 3 +++
> 3 files changed, 7 insertions(+)
>
> diff --git a/fs/qnx4/dir.c b/fs/qnx4/dir.c
> index a6ee23aadd28..11aaf59f0411 100644
> --- a/fs/qnx4/dir.c
> +++ b/fs/qnx4/dir.c
> @@ -31,6 +31,8 @@ static int qnx4_readdir(struct file *file, struct dir_context *ctx)
>
> while (ctx->pos < inode->i_size) {
> blknum = qnx4_block_map(inode, ctx->pos >> QNX4_BLOCK_SIZE_BITS);
> + if (blknum == -EIO)
> + return -EIO;
Since 'blknum' is 'unsigned long' doesn't this generate a compiler
warning about the condition being always false?
(C requires the -EIO be converted to the equivalent unsigned
bit-pattern - but that doesn't stop the compiler deciding it is
dubious.)
If it doesn't this week, it might next week.
What about other error codes that might get returned.
Someone seeing that EIO is valid might decide an other
error can be returned.
You probably ought to allow for all errno values
or use ~0ull as an error value.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
Thanks David,
Please take a look at v3 patch to see if it makes more sense.
Thanks,
- Tong
qnx4_block_map() may return -EIO on funny qnx4 fs image, in this case do
not interpret -EIO as a correct block number
Signed-off-by: Tong Zhang <[email protected]>
---
v2: also check other callers according to Anders Larsen's<[email protected]> comment
v3: change error code from EIO to ~0ull to avoid potential compiler
warning on signed/unsigned comparison
fs/qnx4/dir.c | 2 ++
fs/qnx4/inode.c | 6 ++++--
fs/qnx4/namei.c | 3 +++
3 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/fs/qnx4/dir.c b/fs/qnx4/dir.c
index a6ee23aadd28..0ff7b9f6a887 100644
--- a/fs/qnx4/dir.c
+++ b/fs/qnx4/dir.c
@@ -31,6 +31,8 @@ static int qnx4_readdir(struct file *file, struct dir_context *ctx)
while (ctx->pos < inode->i_size) {
blknum = qnx4_block_map(inode, ctx->pos >> QNX4_BLOCK_SIZE_BITS);
+ if (blknum == ~0ull)
+ return -EIO;
bh = sb_bread(inode->i_sb, blknum);
if (bh == NULL) {
printk(KERN_ERR "qnx4_readdir: bread failed (%ld)\n", blknum);
diff --git a/fs/qnx4/inode.c b/fs/qnx4/inode.c
index e8da1cde87b9..b4fff2db6c7e 100644
--- a/fs/qnx4/inode.c
+++ b/fs/qnx4/inode.c
@@ -59,6 +59,8 @@ static int qnx4_get_block( struct inode *inode, sector_t iblock, struct buffer_h
QNX4DEBUG((KERN_INFO "qnx4: qnx4_get_block inode=[%ld] iblock=[%ld]\n",inode->i_ino,iblock));
phys = qnx4_block_map( inode, iblock );
+ if (phys == ~0ull)
+ return -EIO;
if ( phys ) {
// logical block is before EOF
map_bh(bh, inode->i_sb, phys);
@@ -98,12 +100,12 @@ unsigned long qnx4_block_map( struct inode *inode, long iblock )
bh = sb_bread(inode->i_sb, i_xblk - 1);
if ( !bh ) {
QNX4DEBUG((KERN_ERR "qnx4: I/O error reading xtnt block [%ld])\n", i_xblk - 1));
- return -EIO;
+ return ~0ull;
}
xblk = (struct qnx4_xblk*)bh->b_data;
if ( memcmp( xblk->xblk_signature, "IamXblk", 7 ) ) {
QNX4DEBUG((KERN_ERR "qnx4: block at %ld is not a valid xtnt\n", qnx4_inode->i_xblk));
- return -EIO;
+ return ~0ull;
}
}
block = try_extent(&xblk->xblk_xtnts[ix], &offset);
diff --git a/fs/qnx4/namei.c b/fs/qnx4/namei.c
index 8d72221735d7..c665ba730c11 100644
--- a/fs/qnx4/namei.c
+++ b/fs/qnx4/namei.c
@@ -66,6 +66,8 @@ static struct buffer_head *qnx4_find_entry(int len, struct inode *dir,
while (blkofs * QNX4_BLOCK_SIZE + offset < dir->i_size) {
if (!bh) {
block = qnx4_block_map(dir, blkofs);
+ if (block == ~0ull)
+ goto out;
if (block)
bh = sb_bread(dir->i_sb, block);
if (!bh) {
@@ -88,6 +90,7 @@ static struct buffer_head *qnx4_find_entry(int len, struct inode *dir,
blkofs++;
}
brelse(bh);
+out:
*res_dir = NULL;
return NULL;
}
--
2.25.1
From: Tong Zhang
> Sent: 02 November 2020 23:14
>
> qnx4_block_map() may return -EIO on funny qnx4 fs image, in this case do
> not interpret -EIO as a correct block number
That commit message is now wrong.
Also 'blknum' is only 'unsigned long' so ~0ull is wrong.
It can be worth injecting an error and checking the error
propagation works.
What is the actual maximum file size?
Is there actually an 'out of range' blknum value
that can be used to signify an error without
changing the return value to 'long long'.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
On Tue, Nov 3, 2020 at 5:54 AM David Laight <[email protected]> wrote:
>
> From: Tong Zhang
> > Sent: 02 November 2020 23:14
> >
> > qnx4_block_map() may return -EIO on funny qnx4 fs image, in this case do
> > not interpret -EIO as a correct block number
>
> That commit message is now wrong.
ouch -- what would you suggest here?
> Also 'blknum' is only 'unsigned long' so ~0ull is wrong.
> It can be worth injecting an error and checking the error
> propagation works.
>
> What is the actual maximum file size?
The maximum file size is 2GB-1, but from my understanding
qnx4_block_map() returns
a physical block number. The max disk size supported is 2**64 bytes, however
it is limited to unsigned long (2**32)
-- so I am actually very hesitant to encode an error code in the return value
without changing the function return type, which will introduce more
changes I don't like.
The original -EIO in qnx4_block_map() is also dodgy btw.
> Is there actually an 'out of range' blknum value
> that can be used to signify an error without
> changing the return value to 'long long'.
>
> David
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
>
qnx4_block_map() may return -EIO on funny qnx4 fs image, in this case do
not interpret error as a valid block number.
Signed-off-by: Tong Zhang <[email protected]>
---
v2: also check other callers according to Anders Larsen's<[email protected]> comment
v3: change error code from EIO to ~0ull to avoid potential compiler
warning on signed/unsigned comparison
v4: revert error code back to -EIO and dedicate return value for error code. Also
print a message to let user know there is an error.
fs/qnx4/dir.c | 5 ++++-
fs/qnx4/inode.c | 14 +++++++++-----
fs/qnx4/namei.c | 6 +++++-
fs/qnx4/qnx4.h | 2 +-
4 files changed, 19 insertions(+), 8 deletions(-)
diff --git a/fs/qnx4/dir.c b/fs/qnx4/dir.c
index a6ee23aadd28..49ccd7ddd83b 100644
--- a/fs/qnx4/dir.c
+++ b/fs/qnx4/dir.c
@@ -25,12 +25,15 @@ static int qnx4_readdir(struct file *file, struct dir_context *ctx)
unsigned long blknum;
int ix, ino;
int size;
+ int result;
QNX4DEBUG((KERN_INFO "qnx4_readdir:i_size = %ld\n", (long) inode->i_size));
QNX4DEBUG((KERN_INFO "pos = %ld\n", (long) ctx->pos));
while (ctx->pos < inode->i_size) {
- blknum = qnx4_block_map(inode, ctx->pos >> QNX4_BLOCK_SIZE_BITS);
+ result = qnx4_block_map(inode, ctx->pos >> QNX4_BLOCK_SIZE_BITS, &blknum);
+ if (result)
+ return result;
bh = sb_bread(inode->i_sb, blknum);
if (bh == NULL) {
printk(KERN_ERR "qnx4_readdir: bread failed (%ld)\n", blknum);
diff --git a/fs/qnx4/inode.c b/fs/qnx4/inode.c
index e8da1cde87b9..3333a4ef65fe 100644
--- a/fs/qnx4/inode.c
+++ b/fs/qnx4/inode.c
@@ -54,11 +54,14 @@ static int qnx4_remount(struct super_block *sb, int *flags, char *data)
static int qnx4_get_block( struct inode *inode, sector_t iblock, struct buffer_head *bh, int create )
{
+ int result;
unsigned long phys;
QNX4DEBUG((KERN_INFO "qnx4: qnx4_get_block inode=[%ld] iblock=[%ld]\n",inode->i_ino,iblock));
- phys = qnx4_block_map( inode, iblock );
+ result = qnx4_block_map(inode, iblock, &phys);
+ if (result)
+ return result;
if ( phys ) {
// logical block is before EOF
map_bh(bh, inode->i_sb, phys);
@@ -75,7 +78,7 @@ static inline u32 try_extent(qnx4_xtnt_t *extent, u32 *offset)
return 0;
}
-unsigned long qnx4_block_map( struct inode *inode, long iblock )
+int qnx4_block_map(struct inode *inode, long iblock , unsigned long *phys)
{
int ix;
long i_xblk;
@@ -97,12 +100,12 @@ unsigned long qnx4_block_map( struct inode *inode, long iblock )
// read next xtnt block.
bh = sb_bread(inode->i_sb, i_xblk - 1);
if ( !bh ) {
- QNX4DEBUG((KERN_ERR "qnx4: I/O error reading xtnt block [%ld])\n", i_xblk - 1));
+ printk(KERN_ERR "qnx4: I/O error reading xtnt block [%ld])\n", i_xblk - 1);
return -EIO;
}
xblk = (struct qnx4_xblk*)bh->b_data;
if ( memcmp( xblk->xblk_signature, "IamXblk", 7 ) ) {
- QNX4DEBUG((KERN_ERR "qnx4: block at %ld is not a valid xtnt\n", qnx4_inode->i_xblk));
+ printk(KERN_ERR "qnx4: block at %d is not a valid xtnt\n", qnx4_inode->di_xblk);
return -EIO;
}
}
@@ -123,7 +126,8 @@ unsigned long qnx4_block_map( struct inode *inode, long iblock )
}
QNX4DEBUG((KERN_INFO "qnx4: mapping block %ld of inode %ld = %ld\n",iblock,inode->i_ino,block));
- return block;
+ *phys = block;
+ return 0;
}
static int qnx4_statfs(struct dentry *dentry, struct kstatfs *buf)
diff --git a/fs/qnx4/namei.c b/fs/qnx4/namei.c
index 8d72221735d7..3d64b34dbe6e 100644
--- a/fs/qnx4/namei.c
+++ b/fs/qnx4/namei.c
@@ -59,13 +59,16 @@ static struct buffer_head *qnx4_find_entry(int len, struct inode *dir,
{
unsigned long block, offset, blkofs;
struct buffer_head *bh;
+ int result;
*res_dir = NULL;
bh = NULL;
block = offset = blkofs = 0;
while (blkofs * QNX4_BLOCK_SIZE + offset < dir->i_size) {
if (!bh) {
- block = qnx4_block_map(dir, blkofs);
+ result = qnx4_block_map(dir, blkofs, &block);
+ if (result)
+ goto out;
if (block)
bh = sb_bread(dir->i_sb, block);
if (!bh) {
@@ -88,6 +91,7 @@ static struct buffer_head *qnx4_find_entry(int len, struct inode *dir,
blkofs++;
}
brelse(bh);
+out:
*res_dir = NULL;
return NULL;
}
diff --git a/fs/qnx4/qnx4.h b/fs/qnx4/qnx4.h
index 6283705466a4..efa76aa551fc 100644
--- a/fs/qnx4/qnx4.h
+++ b/fs/qnx4/qnx4.h
@@ -24,7 +24,7 @@ struct qnx4_inode_info {
extern struct inode *qnx4_iget(struct super_block *, unsigned long);
extern struct dentry *qnx4_lookup(struct inode *dir, struct dentry *dentry, unsigned int flags);
extern unsigned long qnx4_count_free_blocks(struct super_block *sb);
-extern unsigned long qnx4_block_map(struct inode *inode, long iblock);
+extern int qnx4_block_map(struct inode *inode, long iblock, unsigned long* phys);
extern const struct inode_operations qnx4_dir_inode_operations;
extern const struct file_operations qnx4_dir_operations;
--
2.25.1
From: Tong Zhang
> Sent: 03 November 2020 13:53
...
> > Also 'blknum' is only 'unsigned long' so ~0ull is wrong.
> > It can be worth injecting an error and checking the error
> > propagation works.
> >
> > What is the actual maximum file size?
> The maximum file size is 2GB-1, but from my understanding
> qnx4_block_map() returns a physical block number.
> The max disk size supported is 2**64 bytes, however
> it is limited to unsigned long (2**32)
> -- so I am actually very hesitant to encode an error code in the return value
> without changing the function return type, which will introduce more
> changes I don't like.
> The original -EIO in qnx4_block_map() is also dodgy btw.
You've put your hand into a bag of worms...
Looks like a load of the 'long' need to be 64bit.
I'm actually surprised how often 'long' appears in the
linux kernel.
I don't believe there was ever a 16bit version (eg 286)
where 'long' was 32bit.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
On Tue, Nov 3, 2020 at 4:57 PM David Laight <[email protected]> wrote:
> You've put your hand into a bag of worms...
That's why I'd prefer minimal changes if possible at all.
Hi,
I was wondering if it is still possible to get this fixed.
I am curious what is the acceptable way to fix this?
Thanks,
- Tong