Hi,
I am writing a linux kernel driver for a new device.
Is it a good practice to set variable NULL after freeing it?
Something like this -
kfree(x);
x = NULL;
Please let me know.
Amit
On Sat, Nov 12, 2022 at 11:47:39AM +0530, A wrote:
> Hi,
>
> I am writing a linux kernel driver for a new device.
>
> Is it a good practice to set variable NULL after freeing it?
>
> Something like this -
>
> kfree(x);
> x = NULL;
>
> Please let me know.
It depends. What's important is not to let a pointer exist to a freed
location, so if you're doing:
kfree(card->pool);
then it's usually important to follow this by:
card->pool = NULL;
so that no code stumbles upon that rotten pointer. Similarly if you're
freeing a variable in the middle of a function or inside an "if" block,
it's wise to reset the pointer so that it doesn't continue to exist
further in the function, and is visually easier to track for anyone
reviewing that code.
But a lot of kfree() calls exist in return paths and are only followed by
other kfree() and a return statement. In this case, it can be completely
useless to NULL a local variable that was just freed as the variable
stops existing when returning. For example, below nullifying the pointers
wouldn't bring anything:
kfree(card->pool);
kfree(card);
return -EBUSY;
A good way to check common practices is to check with git grep how
current code already deals with this:
$ git grep -wA3 kfree
You'll notice that all practices indeed exist.
Willy
PS: you could more easily get responses in the future by associating a
real name with your address instead of just "A" which makes it look
like a spam.
>
> It depends. What's important is not to let a pointer exist to a freed
> location, so if you're doing:
>
> kfree(card->pool);
>
> then it's usually important to follow this by:
>
> card->pool = NULL;
>
I checked in kernel but at many places this is not being done. I can
change all that code. But, will the patch be accepted?
So, if someone is doing -
kfree(x)
._some_code_
._some_code_
._some_code_
Then I can change it to -
kfree(x)
x = NULL;
._some_code_
._some_code_
._some_code_
But, will the patch be accepted for this change?
Please let me know.
Amit
On Sat, Nov 12, 2022 at 04:18:37PM +0530, A wrote:
> >
> > It depends. What's important is not to let a pointer exist to a freed
> > location, so if you're doing:
> >
> > kfree(card->pool);
> >
> > then it's usually important to follow this by:
> >
> > card->pool = NULL;
> >
>
> I checked in kernel but at many places this is not being done. I can
> change all that code. But, will the patch be accepted?
>
> So, if someone is doing -
>
> kfree(x)
> ._some_code_
> ._some_code_
> ._some_code_
>
> Then I can change it to -
>
> kfree(x)
> x = NULL;
> ._some_code_
> ._some_code_
> ._some_code_
>
> But, will the patch be accepted for this change?
I don't think so, for the reasons I explained previously,
unless you spot real bugs there during your reviews, of
course. Better focus on your own driver for now in my
opinion.
Willy
On Sat, Nov 12, 2022 at 05:28:04PM +0530, A wrote:
> I was just thinking that when this is good practice (where its usage
> is genuine), then why is there not a kernel wide macro that would call
> kfree(x) and then set (x) = NULL. So, this will be done automatically
> for everyone and the developer will not have to decide whether to do
> this or not.
Very likely because developers want to decide.
Willy
On Sat, Nov 12, 2022 at 5:31 PM Willy Tarreau <[email protected]> wrote:
>
> On Sat, Nov 12, 2022 at 05:28:04PM +0530, A wrote:
> > I was just thinking that when this is good practice (where its usage
> > is genuine), then why is there not a kernel wide macro that would call
> > kfree(x) and then set (x) = NULL. So, this will be done automatically
> > for everyone and the developer will not have to decide whether to do
> > this or not.
>
> Very likely because developers want to decide.
>
> Willy
Yeah, may be they want to save the time the kernel will spend in doing
(x) = NULL.
Amit
On Sat, Nov 12, 2022 at 5:04 PM Willy Tarreau <[email protected]> wrote:
>
> On Sat, Nov 12, 2022 at 04:18:37PM +0530, A wrote:
> > >
> > > It depends. What's important is not to let a pointer exist to a freed
> > > location, so if you're doing:
> > >
> > > kfree(card->pool);
> > >
> > > then it's usually important to follow this by:
> > >
> > > card->pool = NULL;
> > >
> >
> > I checked in kernel but at many places this is not being done. I can
> > change all that code. But, will the patch be accepted?
> >
> > So, if someone is doing -
> >
> > kfree(x)
> > ._some_code_
> > ._some_code_
> > ._some_code_
> >
> > Then I can change it to -
> >
> > kfree(x)
> > x = NULL;
> > ._some_code_
> > ._some_code_
> > ._some_code_
> >
> > But, will the patch be accepted for this change?
>
> I don't think so, for the reasons I explained previously,
> unless you spot real bugs there during your reviews, of
> course. Better focus on your own driver for now in my
> opinion.
>
I was just thinking that when this is good practice (where its usage
is genuine), then why is there not a kernel wide macro that would call
kfree(x) and then set (x) = NULL. So, this will be done automatically
for everyone and the developer will not have to decide whether to do
this or not.
Amit