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

PATCH In review Chen Gang 2 participants 6 messages Aug 20, 2013 – Sep 3, 2013

Subsystems (2)

Taskstats statistics interface Per-task delay accounting

lore

Share & tools

lore

Chen Gang

author

2013-08-20 02:41 UTC #0 lore raw

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

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 <[email protected]>

---
 kernel/delayacct.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

kernel/delayacct.c +10 -0

index d473988..10cddce 100644
@@ -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,

Signature · 1 line

1.7.7.6

Chen Gang

author

2013-08-20 02:45 UTC #1 lore raw

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

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 <[email protected]>

---
 kernel/taskstats.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

kernel/taskstats.c +6 -2

index 145bb4d..1db6808 100644
@@ -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);

Signature · 1 line

1.7.7.6

Chen Gang

author

2013-09-03 05:08 UTC #2 lore raw

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

Hello maintainers:

Please help check this patch, when you have time.

Thanks.

Quoted 31 lines · 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 <[email protected]>

---
 kernel/taskstats.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

kernel/taskstats.c +6 -2

index 145bb4d..1db6808 100644
@@ -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);

Signature · 1 line

Chen Gang

Andrew Morton

2013-08-21 22:10 UTC #3 lore raw

In reply to Chen Gang

Quoted 3 lines · On Tue, 20 Aug 2013 10:40:03 +0800 Chen Gang <[email protected]> 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()?

Chen Gang

author

2013-08-22 02:51 UTC #4 lore raw

Quoted 6 lines · Andrew Morton

Quoted 3 lines · On Tue, 20 Aug 2013 10:40:03 +0800 Chen Gang <[email protected]> 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.

Quoted 8 lines

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--------------------------------

kernel/delayacct.c +0 -6

index d473988..da8b153 100644
@@ -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.

Signature · 1 line

Chen Gang

Chen Gang

author

2013-09-03 05:04 UTC #5 lore raw

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

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 <[email protected]>

---
 kernel/delayacct.c |    7 -------
 1 files changed, 0 insertions(+), 7 deletions(-)

kernel/delayacct.c +0 -7

index d473988..54996b7 100644
@@ -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;
 }

Signature · 1 line

1.7.7.6