[PATCH] kernel/delayacct.c: check NULL for member variable 'delays' in all extern functions.

[PATCH] kernel/delayacct.c: check NULL for member variable 'delays' in all extern functions.

[Chen Gang]

The member variable 'delays' may be NULL, so need check NULL before use
it for all extern functions, just like __delayacct_tsk_init() and
__delayacct_add_tsk() have already done.


Signed-off-by: Chen Gang <gang.chen@asianux.com>
---
 kernel/delayacct.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/kernel/delayacct.c b/kernel/delayacct.c
index d473988..10cddce 100644
--- a/kernel/delayacct.c
+++ b/kernel/delayacct.c
@@ -81,11 +81,15 @@ static void delayacct_end(struct timespec *start, struct timespec *end,
 
 void __delayacct_blkio_start(void)
 {
+	if (!current->delays)
+		return;
 	delayacct_start(&current->delays->blkio_start);
 }
 
 void __delayacct_blkio_end(void)
 {
+	if (!current->delays)
+		return;
 	if (current->delays->flags & DELAYACCT_PF_SWAPIN)
 		/* Swapin block I/O */
 		delayacct_end(&current->delays->blkio_start,
@@ -167,6 +171,8 @@ __u64 __delayacct_blkio_ticks(struct task_struct *tsk)
 	__u64 ret;
 	unsigned long flags;
 
+	if (!tsk->delays)
+		return 0;
 	spin_lock_irqsave(&tsk->delays->lock, flags);
 	ret = nsec_to_clock_t(tsk->delays->blkio_delay +
 				tsk->delays->swapin_delay);
@@ -176,11 +182,15 @@ __u64 __delayacct_blkio_ticks(struct task_struct *tsk)
 
 void __delayacct_freepages_start(void)
 {
+	if (!current->delays)
+		return;
 	delayacct_start(&current->delays->freepages_start);
 }
 
 void __delayacct_freepages_end(void)
 {
+	if (!current->delays)
+		return;
 	delayacct_end(&current->delays->freepages_start,
 			&current->delays->freepages_end,
 			&current->delays->freepages_delay,
-- 
1.7.7.6

[PATCH] kernel/taskstats.c: add nla_nest_cancel() for failure processing between nla_nest_start() and nla_nest_end()

[Chen Gang]

When failure occurs between nla_nest_start() and nla_nest_end(), need
call nla_nest_cancel() to clean up related things.

Signed-off-by: Chen Gang <gang.chen@asianux.com>
---
 kernel/taskstats.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/kernel/taskstats.c b/kernel/taskstats.c
index 145bb4d..1db6808 100644
--- a/kernel/taskstats.c
+++ b/kernel/taskstats.c
@@ -404,11 +404,15 @@ static struct taskstats *mk_reply(struct sk_buff *skb, int type, u32 pid)
 	if (!na)
 		goto err;
 
-	if (nla_put(skb, type, sizeof(pid), &pid) < 0)
+	if (nla_put(skb, type, sizeof(pid), &pid) < 0) {
+		nla_nest_cancel(skb, na);
 		goto err;
+	}
 	ret = nla_reserve(skb, TASKSTATS_TYPE_STATS, sizeof(struct taskstats));
-	if (!ret)
+	if (!ret) {
+		nla_nest_cancel(skb, na);
 		goto err;
+	}
 	nla_nest_end(skb, na);
 
 	return nla_data(ret);
-- 
1.7.7.6

Re: [PATCH] kernel/delayacct.c: check NULL for member variable 'delays' in all extern functions.

[Andrew Morton]

On Tue, 20 Aug 2013 10:40:03 +0800 Chen Gang <gang.chen@asianux.com> wrote:

> The member variable 'delays' may be NULL, so need check NULL before use
> it for all extern functions, just like __delayacct_tsk_init() and
> __delayacct_add_tsk() have already done.

If task.delays is NULL, the kernel will oops.  I don't recall seeing
such oops reports hence I suspect that you missed some check somewhere.

For example, delayacct_blkio_start() checks current->delays before
calling __delayacct_blkio_start(), so why retest current->delays in
__delayacct_blkio_start()?

Re: [PATCH] kernel/delayacct.c: check NULL for member variable 'delays' in all extern functions.

[Chen Gang]

On 08/22/2013 06:10 AM, Andrew Morton wrote:
> On Tue, 20 Aug 2013 10:40:03 +0800 Chen Gang <gang.chen@asianux.com> wrote:
> 
>> The member variable 'delays' may be NULL, so need check NULL before use
>> it for all extern functions, just like __delayacct_tsk_init() and
>> __delayacct_add_tsk() have already done.
> 
> If task.delays is NULL, the kernel will oops.  I don't recall seeing
> such oops reports hence I suspect that you missed some check somewhere.
> 
> For example, delayacct_blkio_start() checks current->delays before
> calling __delayacct_blkio_start(), so why retest current->delays in
> __delayacct_blkio_start()?
> 
> 

Hmm... Can we assume "__*" extern function is treated as "should not be
used by outside" ?

If so, the original implementation have done (check NULL before use it
for all extern functions), just like your valuable example, thanks.


Hmm... and better to remove the useless code in __delayacct_add_tsk(),
which the related wrapper function has done, the diff is below:

------------------------------diff begin--------------------------------

diff --git a/kernel/delayacct.c b/kernel/delayacct.c
index d473988..da8b153 100644
--- a/kernel/delayacct.c
+++ b/kernel/delayacct.c
@@ -108,12 +108,6 @@ int __delayacct_add_tsk(struct taskstats *d, struct task_struct *tsk)
 	struct timespec ts;
 	cputime_t utime, stime, stimescaled, utimescaled;
 
-	/* Though tsk->delays accessed later, early exit avoids
-	 * unnecessary returning of other data
-	 */
-	if (!tsk->delays)
-		goto done;
-
 	tmp = (s64)d->cpu_run_real_total;
 	task_cputime(tsk, &utime, &stime);
 	cputime_to_timespec(utime + stime, &ts);

------------------------------diff end----------------------------------

If this diff is OK, I will send related patch for it.


Thanks.
-- 
Chen Gang

[PATCH] kernel/delayacct.c: remove redundancy checking in __delayacct_add_tsk()

[Chen Gang]

The wrapper function delayacct_add_tsk() already checked 'tsk->delays',
and __delayacct_add_tsk() has no another direct callers, so can remove
the redundancy checking code.

And the label 'done' is also useless, so remove it, too.


Signed-off-by: Chen Gang <gang.chen@asianux.com>
---
 kernel/delayacct.c |    7 -------
 1 files changed, 0 insertions(+), 7 deletions(-)

diff --git a/kernel/delayacct.c b/kernel/delayacct.c
index d473988..54996b7 100644
--- a/kernel/delayacct.c
+++ b/kernel/delayacct.c
@@ -108,12 +108,6 @@ int __delayacct_add_tsk(struct taskstats *d, struct task_struct *tsk)
 	struct timespec ts;
 	cputime_t utime, stime, stimescaled, utimescaled;
 
-	/* Though tsk->delays accessed later, early exit avoids
-	 * unnecessary returning of other data
-	 */
-	if (!tsk->delays)
-		goto done;
-
 	tmp = (s64)d->cpu_run_real_total;
 	task_cputime(tsk, &utime, &stime);
 	cputime_to_timespec(utime + stime, &ts);
@@ -158,7 +152,6 @@ int __delayacct_add_tsk(struct taskstats *d, struct task_struct *tsk)
 	d->freepages_count += tsk->delays->freepages_count;
 	spin_unlock_irqrestore(&tsk->delays->lock, flags);
 
-done:
 	return 0;
 }
 
-- 
1.7.7.6

Re: [PATCH] kernel/taskstats.c: add nla_nest_cancel() for failure processing between nla_nest_start() and nla_nest_end()

[Chen Gang]

Hello maintainers:

Please help check this patch, when you have time.

Thanks.

On 08/20/2013 10:44 AM, Chen Gang wrote:
> When failure occurs between nla_nest_start() and nla_nest_end(), need
> call nla_nest_cancel() to clean up related things.
> 
> Signed-off-by: Chen Gang <gang.chen@asianux.com>
> ---
>  kernel/taskstats.c |    8 ++++++--
>  1 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/taskstats.c b/kernel/taskstats.c
> index 145bb4d..1db6808 100644
> --- a/kernel/taskstats.c
> +++ b/kernel/taskstats.c
> @@ -404,11 +404,15 @@ static struct taskstats *mk_reply(struct sk_buff *skb, int type, u32 pid)
>  	if (!na)
>  		goto err;
>  
> -	if (nla_put(skb, type, sizeof(pid), &pid) < 0)
> +	if (nla_put(skb, type, sizeof(pid), &pid) < 0) {
> +		nla_nest_cancel(skb, na);
>  		goto err;
> +	}
>  	ret = nla_reserve(skb, TASKSTATS_TYPE_STATS, sizeof(struct taskstats));
> -	if (!ret)
> +	if (!ret) {
> +		nla_nest_cancel(skb, na);
>  		goto err;
> +	}
>  	nla_nest_end(skb, na);
>  
>  	return nla_data(ret);
> 


-- 
Chen Gang