login
Search
This patch adds cheking for granted memory while
filling up inode data to prevent possible NULL
pointer usage. If there is not enough memory to
fill inode data we just mark it as "bad".
Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
Please check the patch, maybe just marking inode as
"bad" is not a good solution.
fs/udf/inode.c | 27 ++++++++++++++++++++++++---
1 files changed, 24 insertions(+), 3 deletions(-)
diff --git a/fs/udf/inode.c b/fs/udf/inode.c
index c846155..91cddae 100644
--- a/fs/udf/inode.c
+++ b/fs/udf/inode.c
@@ -1144,6 +1144,13 @@ static void udf_fill_inode(struct inode *inode, struct buffer_head *bh)
UDF_I_EFE(inode) = 1;
UDF_I_USE(inode) = 0;
UDF_I_DATA(inode) = kmalloc(inode->i_sb->s_blocksize - sizeof(struct extendedFileEntry), GFP_KERNEL);
+ if (!UDF_I_DATA(inode))
+ {
+ printk(KERN_ERR "udf: udf_fill_inode(ino %ld) no free memory\n",
+ inode->i_ino);
+ make_bad_inode(inode);
+ return;
+ }
memcpy(UDF_I_DATA(inode), bh->b_data + sizeof(struct extendedFileEntry), inode->i_sb->s_blocksize - sizeof(struct extendedFileEntry));
}
else if (le16_to_cpu(fe->descTag.tagIdent) == TAG_IDENT_FE)
@@ -1151,6 +1158,13 @@ static void udf_fill_inode(struct inode *inode, struct buffer_head *bh)
UDF_I_EFE(inode) = 0;
UDF_I_USE(inode) = 0;
UDF_I_DATA(inode) = kmalloc(inode->i_sb->s_blocksize - sizeof(struct fileEntry), GFP_KERNEL);
+ if (!UDF_I_DATA(inode))
+ {
+ printk(KERN_ERR "udf: udf_fill_inode(ino %ld) no free memory\n",
+ inode->i_ino);
+ make_bad_inode(inode);
+ return;
+ }
memcpy(UDF_I_DATA(inode), bh->b_data + sizeof(struct fileEntry), inode->i_sb->s_blocksize - sizeof(struct fileEntry));
}
else if (le16_to_cpu(fe->descTag.tagIdent) == TAG_IDENT_USE)
@@ -1161,6 +1175,13 @@ static void udf_fill_inode(struct inode *inode, struct buffer_head *bh)
le32_to_cpu(
((struct unallocSpaceEntry *)bh->b_data)->lengthAllocDescs);
UDF_I_DATA(inode) = ...On Thu, 10 May 2007 18:00:00 +0400
But please let's not add three copies of identical code. Do something like:
static int udf_check_inode(struct inode *inode)
{
if (!UDF_I_DATA(inode)) {
printk(KERN_ERR "udf: udf_fill_inode(ino %ld) no free memory\n",
inode->i_ino);
make_bad_inode(inode);
return -1;
}
return 0;
}
if (udf_check_inode(inode))
return;
In fact you can also do the kmalloc in that helper function too:
static int udf_alloc_i_data(struct inode *inode, size_t size)
{
UDF_I_DATA(inode) = kmalloc(...);
...
}
-
[Andrew Morton - Thu, May 10, 2007 at 03:46:40PM -0700] [...snip...] | But please let's not add three copies of identical code. Do something like: [...snip...] Thanks for comments, Andrew. Let me rewrite the patch... Cyrill -
And please get rid of the UDF_I_* macro for everything you touch, just put a struct udf_inode_info *uip = UDF_I(inode); at the beginning of the function and use the fields directly. -
[Christoph Hellwig - Fri, May 11, 2007 at 08:29:39AM +0100]
| On Thu, May 10, 2007 at 03:46:40PM -0700, Andrew Morton wrote:
| > On Thu, 10 May 2007 18:00:00 +0400
| > Cyrill Gorcunov <gorcunov@gmail.com> wrote:
| >
| > > This patch adds cheking for granted memory while
| > > filling up inode data to prevent possible NULL
| > > pointer usage. If there is not enough memory to
| > > fill inode data we just mark it as "bad".
| > >
| > > Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
| > >
| > > Please check the patch, maybe just marking inode as
| > > "bad" is not a good solution.
| > >
| >
| > yes, make_bad_inode() is appropriate here.
| >
| > >
| > > diff --git a/fs/udf/inode.c b/fs/udf/inode.c
| > > index c846155..91cddae 100644
| > > --- a/fs/udf/inode.c
| > > +++ b/fs/udf/inode.c
| > > @@ -1144,6 +1144,13 @@ static void udf_fill_inode(struct inode *inode, struct buffer_head *bh)
| > > UDF_I_EFE(inode) = 1;
| > > UDF_I_USE(inode) = 0;
| > > UDF_I_DATA(inode) = kmalloc(inode->i_sb->s_blocksize - sizeof(struct extendedFileEntry), GFP_KERNEL);
| > > + if (!UDF_I_DATA(inode))
| > > + {
| > > + printk(KERN_ERR "udf: udf_fill_inode(ino %ld) no free memory\n",
| > > + inode->i_ino);
| > > + make_bad_inode(inode);
| > > + return;
| > > + }
| >
| > But please let's not add three copies of identical code. Do something like:
| >
| > static int udf_check_inode(struct inode *inode)
| > {
| > if (!UDF_I_DATA(inode)) {
| > printk(KERN_ERR "udf: udf_fill_inode(ino %ld) no free memory\n",
| > inode->i_ino);
| > make_bad_inode(inode);
| > return -1;
| > }
| > return 0;
| > }
| >
| >
| > if (udf_check_inode(inode))
| > return;
| >
| > In fact you can also do the kmalloc in that helper function too:
| >
| > static int udf_alloc_i_data(struct inode *inode, size_t size)
| > {
| > UDF_I_DATA(inode) = kmalloc(...);
| > ...
| > }
|
| And please get rid of the UDF_I_* macro for everything you touch, just
| put ...[Christoph Hellwig - Fri, May 11, 2007 at 08:29:39AM +0100] [..snip..] | And please get rid of the UDF_I_* macro for everything you touch, just | put a | | struct udf_inode_info *uip = UDF_I(inode); | | at the beginning of the function and use the fields directly. | Christoph, I think there should be a separated patch to remove UDF_I_* macro. I'll make it ;) Andrew, may be I should produce a series of patch: - 1st to check the memory - 2nd to get rid of UDF_I_* macro ? Cyrill -
[Christoph Hellwig - Fri, May 11, 2007 at 08:29:39AM +0100] ... | And please get rid of the UDF_I_* macro for everything you touch, just | put a | | struct udf_inode_info *uip = UDF_I(inode); | | at the beginning of the function and use the fields directly. | Actually to properly remove UDF_I* and UDF_SB_* macroses in the whole UDF subsystem - is _lot_ of work. I'm going to make it but not now (too busy). Cyrill -
Doing it completely is a lot of work, yes. I was more thinking of converting a piece of code once you do major changes. But if you want to convert all the code as a separate patch I'm more than happy aswell. -
[Christoph Hellwig - Fri, May 11, 2007 at 11:39:56AM +0100] | On Fri, May 11, 2007 at 01:01:27PM +0400, Cyrill Gorcunov wrote: | > [Christoph Hellwig - Fri, May 11, 2007 at 08:29:39AM +0100] | > | > ... | > | > | And please get rid of the UDF_I_* macro for everything you touch, just | > | put a | > | | > | struct udf_inode_info *uip = UDF_I(inode); | > | | > | at the beginning of the function and use the fields directly. | > | | > | > Actually to properly remove UDF_I* and UDF_SB_* macroses in the | > whole UDF subsystem - is _lot_ of work. I'm going to make it but | > not now (too busy). | | Doing it completely is a lot of work, yes. I was more thinking of | converting a piece of code once you do major changes. But if you | want to convert all the code as a separate patch I'm more than happy | aswell. | Christoph, my only argue against getting rid of UDF_I_* macro in my patch is UDF coding style, I don't want to damage it. I think we may leave it as is (including my patch). So just review the patch I sent (second version) and Ack it then so Andrew could include it into mm tree. Meantime I'm rewritting the whole UDF subsystem to get rid of that macroses (it will be a long way ;) Cyrill P.S. But if you still insist on getting rid of UDF_I_ macroses from my patch just let me now :) -
The UDF style is horrible and very unlike other kernel code. Given that udf has been pretty much unmtained for a while there should be nothing in the way of fixing it. Anyway, the patch is technically correct so you'll get my ACK (not that you should need it). -
[Christoph Hellwig - Sun, May 13, 2007 at 10:01:26PM +0100] | On Fri, May 11, 2007 at 03:09:20PM +0400, Cyrill Gorcunov wrote: | > | > | And please get rid of the UDF_I_* macro for everything you touch, just | > | > | put a | > | > | | > | > | struct udf_inode_info *uip = UDF_I(inode); | > | > | | > | > | at the beginning of the function and use the fields directly. | > | > | | > | > | > | > Actually to properly remove UDF_I* and UDF_SB_* macroses in the | > | > whole UDF subsystem - is _lot_ of work. I'm going to make it but | > | > not now (too busy). | > | | > | Doing it completely is a lot of work, yes. I was more thinking of | > | converting a piece of code once you do major changes. But if you | > | want to convert all the code as a separate patch I'm more than happy | > | aswell. | > | | > | > Christoph, my only argue against getting rid of UDF_I_* macro in | > my patch is UDF coding style, I don't want to damage it. I think | > we may leave it as is (including my patch). So just review the patch | > I sent (second version) and Ack it then so Andrew could include it | > into mm tree. Meantime I'm rewritting the whole UDF subsystem to | > get rid of that macroses (it will be a long way ;) | | The UDF style is horrible and very unlike other kernel code. Given | that udf has been pretty much unmtained for a while there should be | nothing in the way of fixing it. | | Anyway, the patch is technically correct so you'll get my ACK (not | that you should need it). | Hi Christoph, you know I've read UDF sources. As I understand all UDF_I_ macroses could be converted without breaking UDF state but... as you exactly mentoined it's style is horrible and I'm thinking about rewritting the whole UDF system. Unfortunelly I'm not _mature_ kernel developer (I'm kernel newbie) and it could take a long time for this (I think something like ~ 3 month or more ;). Actually I'm ready to spend my free time for this. So how do you think could it be ...
I've spent some time hunting bugs in UDF recently so I'll warn you a bit :). Definitely rewriting that ... code would be a good thing to do (reading that code I had urges to do it several times). The hard thing is that there is no reasonable spec you could use - there are two documents which define how UDF should look like but they are really hard to read (they have like hundred pages each and one does not make sence without the other). And reading the code and learning how the filesystem is supposed to work isn't too helpful either. Just a friendly warning ;) Honza -- Jan Kara <jack@suse.cz> SuSE CR Labs -
[Jan Kara - Wed, May 16, 2007 at 07:38:52PM +0200] | > [Christoph Hellwig - Sun, May 13, 2007 at 10:01:26PM +0100] | > | On Fri, May 11, 2007 at 03:09:20PM +0400, Cyrill Gorcunov wrote: | > | > | > | And please get rid of the UDF_I_* macro for everything you touch, just | > | > | > | put a | > | > | > | | > | > | > | struct udf_inode_info *uip = UDF_I(inode); | > | > | > | | > | > | > | at the beginning of the function and use the fields directly. | > | > | > | | > | > | > | > | > | > Actually to properly remove UDF_I* and UDF_SB_* macroses in the | > | > | > whole UDF subsystem - is _lot_ of work. I'm going to make it but | > | > | > not now (too busy). | > | > | | > | > | Doing it completely is a lot of work, yes. I was more thinking of | > | > | converting a piece of code once you do major changes. But if you | > | > | want to convert all the code as a separate patch I'm more than happy | > | > | aswell. | > | > | | > | > | > | > Christoph, my only argue against getting rid of UDF_I_* macro in | > | > my patch is UDF coding style, I don't want to damage it. I think | > | > we may leave it as is (including my patch). So just review the patch | > | > I sent (second version) and Ack it then so Andrew could include it | > | > into mm tree. Meantime I'm rewritting the whole UDF subsystem to | > | > get rid of that macroses (it will be a long way ;) | > | | > | The UDF style is horrible and very unlike other kernel code. Given | > | that udf has been pretty much unmtained for a while there should be | > | nothing in the way of fixing it. | > | | > | Anyway, the patch is technically correct so you'll get my ACK (not | > | that you should need it). | > | | > | > you know I've read UDF sources. As I understand all UDF_I_ macroses | > could be converted without breaking UDF state but... as you exactly | > mentoined it's style is horrible and I'm thinking about rewritting the | > whole UDF system. Unfortunelly I'm not _mature_ kernel developer (I'm kernel | > ...
That's probably a good step. And while converting things to a proper style you'll surely find various bugs and thinkgs you could improve. Write them down somewhere and work on fixing them. And make sure every actualy fix or behaviour change is a single patch separated from the cleanups. -
[Christoph Hellwig - Wed, May 16, 2007 at 06:56:00PM +0100] | On Wed, May 16, 2007 at 09:52:57PM +0400, Cyrill Gorcunov wrote: | > I've that documants even printed ;) Actually they are _very-very_ big | > indeed. I don't know may be just try to bring this code into Linux | > codying style? | | That's probably a good step. And while converting things to a proper | style you'll surely find various bugs and thinkgs you could improve. | Write them down somewhere and work on fixing them. And make sure every | actualy fix or behaviour change is a single patch separated from the | cleanups. | Hi Christoph, I almost have completed UDF style conversation (the only thing to do is to check all I've changed). And I've been striked down by the simple question: the conversion I've done is over 2.6.22-rc1 but meantime in -mm tree two my patches already exist so should I take them into account while converting UDF style? Cyrill -
I guess Andrew would be most grateful if you counted with these two patches... Honza -- Jan Kara <jack@suse.cz> SuSE CR Labs -
It's probably best if you do that patch ontop of -mm so it includes your patches. -
Christoph, you know I've got a question (may be it's stupid) - what a sense to discard UDF_I_* macroses? I mean as I see they don't slow down execution of the code... Cyrill -
Hi Cyrill They make the code harder to read and maintain. -
[Pekka Enberg - Sat, May 12, 2007 at 01:15:14PM +0300] | Hi Cyrill | | On 5/12/07, Cyrill Gorcunov <gorcunov@gmail.com> wrote: | >you know I've got a question (may be it's stupid) - what a | >sense to discard UDF_I_* macroses? I mean as I see they | >don't slow down execution of the code... | | They make the code harder to read and maintain. | Hi Pekka, thanks for eplanation. Btw, maybe I also should convert the UDF coding style to Linux kernel coding style? Cyrill P.S. It's strange that I haven't got any comments from Ben Fennema :( -
