Replace malloc/memset with calloc
Signed-off-by: Gaurav Singh <[email protected]>
---
samples/bpf/xdp_rxq_info_user.c | 13 +++----------
1 file changed, 3 insertions(+), 10 deletions(-)
diff --git a/samples/bpf/xdp_rxq_info_user.c b/samples/bpf/xdp_rxq_info_user.c
index 4fe47502ebed..caa4e7ffcfc7 100644
--- a/samples/bpf/xdp_rxq_info_user.c
+++ b/samples/bpf/xdp_rxq_info_user.c
@@ -198,11 +198,8 @@ static struct datarec *alloc_record_per_cpu(void)
{
unsigned int nr_cpus = bpf_num_possible_cpus();
struct datarec *array;
- size_t size;
- size = sizeof(struct datarec) * nr_cpus;
- array = malloc(size);
- memset(array, 0, size);
+ array = calloc(nr_cpus, sizeof(struct datarec));
if (!array) {
fprintf(stderr, "Mem alloc error (nr_cpus:%u)\n", nr_cpus);
exit(EXIT_FAIL_MEM);
@@ -214,11 +211,8 @@ static struct record *alloc_record_per_rxq(void)
{
unsigned int nr_rxqs = bpf_map__def(rx_queue_index_map)->max_entries;
struct record *array;
- size_t size;
- size = sizeof(struct record) * nr_rxqs;
- array = malloc(size);
- memset(array, 0, size);
+ array = calloc(nr_rxqs, sizeof(struct record));
if (!array) {
fprintf(stderr, "Mem alloc error (nr_rxqs:%u)\n", nr_rxqs);
exit(EXIT_FAIL_MEM);
@@ -232,8 +226,7 @@ static struct stats_record *alloc_stats_record(void)
struct stats_record *rec;
int i;
- rec = malloc(sizeof(*rec));
- memset(rec, 0, sizeof(*rec));
+ rec = calloc(1, sizeof(struct stats_record));
if (!rec) {
fprintf(stderr, "Mem alloc error\n");
exit(EXIT_FAIL_MEM);
--
2.17.1
On Thu, 11 Jun 2020 11:02:21 -0400
Gaurav Singh <[email protected]> wrote:
> Replace malloc/memset with calloc
Please also mention/describe that this also solves the bug you found.
As this fix a potential bug, it will be appropriate to add a "Fixes:"
line, just before "Signed-off-by" (meaning no newline between the two).
Fixes: 0fca931a6f21 ("samples/bpf: program demonstrating access to xdp_rxq_info")
> Signed-off-by: Gaurav Singh <[email protected]>
> ---
> samples/bpf/xdp_rxq_info_user.c | 13 +++----------
> 1 file changed, 3 insertions(+), 10 deletions(-)
>
> diff --git a/samples/bpf/xdp_rxq_info_user.c b/samples/bpf/xdp_rxq_info_user.c
> index 4fe47502ebed..caa4e7ffcfc7 100644
> --- a/samples/bpf/xdp_rxq_info_user.c
> +++ b/samples/bpf/xdp_rxq_info_user.c
> @@ -198,11 +198,8 @@ static struct datarec *alloc_record_per_cpu(void)
> {
> unsigned int nr_cpus = bpf_num_possible_cpus();
> struct datarec *array;
> - size_t size;
>
> - size = sizeof(struct datarec) * nr_cpus;
> - array = malloc(size);
> - memset(array, 0, size);
> + array = calloc(nr_cpus, sizeof(struct datarec));
> if (!array) {
> fprintf(stderr, "Mem alloc error (nr_cpus:%u)\n", nr_cpus);
> exit(EXIT_FAIL_MEM);
> @@ -214,11 +211,8 @@ static struct record *alloc_record_per_rxq(void)
> {
> unsigned int nr_rxqs = bpf_map__def(rx_queue_index_map)->max_entries;
> struct record *array;
> - size_t size;
>
> - size = sizeof(struct record) * nr_rxqs;
> - array = malloc(size);
> - memset(array, 0, size);
> + array = calloc(nr_rxqs, sizeof(struct record));
> if (!array) {
> fprintf(stderr, "Mem alloc error (nr_rxqs:%u)\n", nr_rxqs);
> exit(EXIT_FAIL_MEM);
> @@ -232,8 +226,7 @@ static struct stats_record *alloc_stats_record(void)
> struct stats_record *rec;
> int i;
>
> - rec = malloc(sizeof(*rec));
> - memset(rec, 0, sizeof(*rec));
> + rec = calloc(1, sizeof(struct stats_record));
> if (!rec) {
> fprintf(stderr, "Mem alloc error\n");
> exit(EXIT_FAIL_MEM);
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer
Replace malloc/memset with calloc
Fixes: 0fca931a6f21 ("samples/bpf: program demonstrating access to xdp_rxq_info") Signed-off-by: Gaurav Singh <[email protected]>
---
samples/bpf/xdp_rxq_info_user.c | 13 +++----------
1 file changed, 3 insertions(+), 10 deletions(-)
diff --git a/samples/bpf/xdp_rxq_info_user.c b/samples/bpf/xdp_rxq_info_user.c
index 4fe47502ebed..caa4e7ffcfc7 100644
--- a/samples/bpf/xdp_rxq_info_user.c
+++ b/samples/bpf/xdp_rxq_info_user.c
@@ -198,11 +198,8 @@ static struct datarec *alloc_record_per_cpu(void)
{
unsigned int nr_cpus = bpf_num_possible_cpus();
struct datarec *array;
- size_t size;
- size = sizeof(struct datarec) * nr_cpus;
- array = malloc(size);
- memset(array, 0, size);
+ array = calloc(nr_cpus, sizeof(struct datarec));
if (!array) {
fprintf(stderr, "Mem alloc error (nr_cpus:%u)\n", nr_cpus);
exit(EXIT_FAIL_MEM);
@@ -214,11 +211,8 @@ static struct record *alloc_record_per_rxq(void)
{
unsigned int nr_rxqs = bpf_map__def(rx_queue_index_map)->max_entries;
struct record *array;
- size_t size;
- size = sizeof(struct record) * nr_rxqs;
- array = malloc(size);
- memset(array, 0, size);
+ array = calloc(nr_rxqs, sizeof(struct record));
if (!array) {
fprintf(stderr, "Mem alloc error (nr_rxqs:%u)\n", nr_rxqs);
exit(EXIT_FAIL_MEM);
@@ -232,8 +226,7 @@ static struct stats_record *alloc_stats_record(void)
struct stats_record *rec;
int i;
- rec = malloc(sizeof(*rec));
- memset(rec, 0, sizeof(*rec));
+ rec = calloc(1, sizeof(struct stats_record));
if (!rec) {
fprintf(stderr, "Mem alloc error\n");
exit(EXIT_FAIL_MEM);
--
2.17.1
Replace malloc/memset with calloc
Fixes: 0fca931a6f21 ("samples/bpf: program demonstrating access to xdp_rxq_info")
Signed-off-by: Gaurav Singh <[email protected]>
---
samples/bpf/xdp_rxq_info_user.c | 13 +++----------
1 file changed, 3 insertions(+), 10 deletions(-)
diff --git a/samples/bpf/xdp_rxq_info_user.c b/samples/bpf/xdp_rxq_info_user.c
index 4fe47502ebed..caa4e7ffcfc7 100644
--- a/samples/bpf/xdp_rxq_info_user.c
+++ b/samples/bpf/xdp_rxq_info_user.c
@@ -198,11 +198,8 @@ static struct datarec *alloc_record_per_cpu(void)
{
unsigned int nr_cpus = bpf_num_possible_cpus();
struct datarec *array;
- size_t size;
- size = sizeof(struct datarec) * nr_cpus;
- array = malloc(size);
- memset(array, 0, size);
+ array = calloc(nr_cpus, sizeof(struct datarec));
if (!array) {
fprintf(stderr, "Mem alloc error (nr_cpus:%u)\n", nr_cpus);
exit(EXIT_FAIL_MEM);
@@ -214,11 +211,8 @@ static struct record *alloc_record_per_rxq(void)
{
unsigned int nr_rxqs = bpf_map__def(rx_queue_index_map)->max_entries;
struct record *array;
- size_t size;
- size = sizeof(struct record) * nr_rxqs;
- array = malloc(size);
- memset(array, 0, size);
+ array = calloc(nr_rxqs, sizeof(struct record));
if (!array) {
fprintf(stderr, "Mem alloc error (nr_rxqs:%u)\n", nr_rxqs);
exit(EXIT_FAIL_MEM);
@@ -232,8 +226,7 @@ static struct stats_record *alloc_stats_record(void)
struct stats_record *rec;
int i;
- rec = malloc(sizeof(*rec));
- memset(rec, 0, sizeof(*rec));
+ rec = calloc(1, sizeof(struct stats_record));
if (!rec) {
fprintf(stderr, "Mem alloc error\n");
exit(EXIT_FAIL_MEM);
--
2.17.1
On Thu, 11 Jun 2020 20:36:40 -0400
Gaurav Singh <[email protected]> wrote:
> Replace malloc/memset with calloc
>
> Fixes: 0fca931a6f21 ("samples/bpf: program demonstrating access to xdp_rxq_info")
> Signed-off-by: Gaurav Singh <[email protected]>
Above is the correct use of Fixes + Signed-off-by.
Now you need to update/improve the description, to also
mention/describe that this also solves the bug you found.
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer
On Fri, 2020-06-12 at 08:42 +0200, Jesper Dangaard Brouer wrote:
> On Thu, 11 Jun 2020 20:36:40 -0400
> Gaurav Singh <[email protected]> wrote:
>
> > Replace malloc/memset with calloc
> >
> > Fixes: 0fca931a6f21 ("samples/bpf: program demonstrating access to xdp_rxq_info")
> > Signed-off-by: Gaurav Singh <[email protected]>
>
> Above is the correct use of Fixes + Signed-off-by.
>
> Now you need to update/improve the description, to also
> mention/describe that this also solves the bug you found.
This is not a fix, it's a conversion of one
correct code to a shorter one.
On Fri, 12 Jun 2020 03:14:58 -0700
Joe Perches <[email protected]> wrote:
> On Fri, 2020-06-12 at 08:42 +0200, Jesper Dangaard Brouer wrote:
> > On Thu, 11 Jun 2020 20:36:40 -0400
> > Gaurav Singh <[email protected]> wrote:
> >
> > > Replace malloc/memset with calloc
> > >
> > > Fixes: 0fca931a6f21 ("samples/bpf: program demonstrating access to xdp_rxq_info")
> > > Signed-off-by: Gaurav Singh <[email protected]>
> >
> > Above is the correct use of Fixes + Signed-off-by.
> >
> > Now you need to update/improve the description, to also
> > mention/describe that this also solves the bug you found.
>
> This is not a fix, it's a conversion of one
> correct code to a shorter one.
Read the code again Joe. There is a bug in the code that gets removed,
as it runs memset on the memory before checking if it was NULL.
IHMO this proves why is it is necessary to mention in the commit
message, as you didn't notice the bug in your code review.
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer
Joe Perches <[email protected]> writes:
> On Fri, 2020-06-12 at 08:42 +0200, Jesper Dangaard Brouer wrote:
>> On Thu, 11 Jun 2020 20:36:40 -0400
>> Gaurav Singh <[email protected]> wrote:
>>
>> > Replace malloc/memset with calloc
>> >
>> > Fixes: 0fca931a6f21 ("samples/bpf: program demonstrating access to xdp_rxq_info")
>> > Signed-off-by: Gaurav Singh <[email protected]>
>>
>> Above is the correct use of Fixes + Signed-off-by.
>>
>> Now you need to update/improve the description, to also
>> mention/describe that this also solves the bug you found.
>
> This is not a fix, it's a conversion of one
> correct code to a shorter one.
No it isn't - the original code memset()s before it checks the return
from malloc(), so it's a potential NULL-pointer reference... Which the
commit message should explain, obviously :)
-Toke
On Fri, 2020-06-12 at 14:05 +0200, Jesper Dangaard Brouer wrote:
> On Fri, 12 Jun 2020 03:14:58 -0700
> Joe Perches <[email protected]> wrote:
>
> > On Fri, 2020-06-12 at 08:42 +0200, Jesper Dangaard Brouer wrote:
> > > On Thu, 11 Jun 2020 20:36:40 -0400
> > > Gaurav Singh <[email protected]> wrote:
> > >
> > > > Replace malloc/memset with calloc
> > > >
> > > > Fixes: 0fca931a6f21 ("samples/bpf: program demonstrating access to xdp_rxq_info")
> > > > Signed-off-by: Gaurav Singh <[email protected]>
> > >
> > > Above is the correct use of Fixes + Signed-off-by.
> > >
> > > Now you need to update/improve the description, to also
> > > mention/describe that this also solves the bug you found.
> >
> > This is not a fix, it's a conversion of one
> > correct code to a shorter one.
>
> Read the code again Joe. There is a bug in the code that gets removed,
> as it runs memset on the memory before checking if it was NULL.
>
> IHMO this proves why is it is necessary to mention in the commit
> message, as you didn't notice the bug in your code review.
I didn't review the code at all, just the commit message,
It's important to have commit messages that describe the
defect being corrected too.
Otherwise, a simple malloc/memset(0) vs zalloc equivalent
is not actually a defect.
Memset on the pointer right after malloc can cause a
null pointer deference if it failed to allocate memory.
A simple fix is to replace malloc/memset with a calloc()
Fixes: 0fca931a6f21 ("samples/bpf: program demonstrating access to xdp_rxq_info")
Signed-off-by: Gaurav Singh <[email protected]>
---
samples/bpf/xdp_rxq_info_user.c | 13 +++----------
1 file changed, 3 insertions(+), 10 deletions(-)
diff --git a/samples/bpf/xdp_rxq_info_user.c b/samples/bpf/xdp_rxq_info_user.c
index 4fe47502ebed..caa4e7ffcfc7 100644
--- a/samples/bpf/xdp_rxq_info_user.c
+++ b/samples/bpf/xdp_rxq_info_user.c
@@ -198,11 +198,8 @@ static struct datarec *alloc_record_per_cpu(void)
{
unsigned int nr_cpus = bpf_num_possible_cpus();
struct datarec *array;
- size_t size;
- size = sizeof(struct datarec) * nr_cpus;
- array = malloc(size);
- memset(array, 0, size);
+ array = calloc(nr_cpus, sizeof(struct datarec));
if (!array) {
fprintf(stderr, "Mem alloc error (nr_cpus:%u)\n", nr_cpus);
exit(EXIT_FAIL_MEM);
@@ -214,11 +211,8 @@ static struct record *alloc_record_per_rxq(void)
{
unsigned int nr_rxqs = bpf_map__def(rx_queue_index_map)->max_entries;
struct record *array;
- size_t size;
- size = sizeof(struct record) * nr_rxqs;
- array = malloc(size);
- memset(array, 0, size);
+ array = calloc(nr_rxqs, sizeof(struct record));
if (!array) {
fprintf(stderr, "Mem alloc error (nr_rxqs:%u)\n", nr_rxqs);
exit(EXIT_FAIL_MEM);
@@ -232,8 +226,7 @@ static struct stats_record *alloc_stats_record(void)
struct stats_record *rec;
int i;
- rec = malloc(sizeof(*rec));
- memset(rec, 0, sizeof(*rec));
+ rec = calloc(1, sizeof(struct stats_record));
if (!rec) {
fprintf(stderr, "Mem alloc error\n");
exit(EXIT_FAIL_MEM);
--
2.17.1
On Fri, 12 Jun 2020 14:53:27 -0400
Gaurav Singh <[email protected]> wrote:
> Memset on the pointer right after malloc can cause a
> null pointer deference if it failed to allocate memory.
> A simple fix is to replace malloc/memset with a calloc()
>
> Fixes: 0fca931a6f21 ("samples/bpf: program demonstrating access to xdp_rxq_info")
> Signed-off-by: Gaurav Singh <[email protected]>
Acked-by: Jesper Dangaard Brouer <[email protected]>
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer
Jesper Dangaard Brouer wrote:
> On Fri, 12 Jun 2020 14:53:27 -0400
> Gaurav Singh <[email protected]> wrote:
>
> > Memset on the pointer right after malloc can cause a
> > null pointer deference if it failed to allocate memory.
> > A simple fix is to replace malloc/memset with a calloc()
> >
> > Fixes: 0fca931a6f21 ("samples/bpf: program demonstrating access to xdp_rxq_info")
> > Signed-off-by: Gaurav Singh <[email protected]>
>
> Acked-by: Jesper Dangaard Brouer <[email protected]>
>
Acked-by: John Fastabend <[email protected]>
If the task is NULL and the if condition is true
then task->pid would cause null pointer dereference.
Fix this by adding additional null check.
Signed-off-by: Gaurav Singh <[email protected]>
---
arch/ia64/kernel/perfmon_default_smpl.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/ia64/kernel/perfmon_default_smpl.c b/arch/ia64/kernel/perfmon_default_smpl.c
index a40c56020fc5..2732b4cf4296 100644
--- a/arch/ia64/kernel/perfmon_default_smpl.c
+++ b/arch/ia64/kernel/perfmon_default_smpl.c
@@ -111,7 +111,7 @@ default_handler(struct task_struct *task, void *buf, pfm_ovfl_arg_t *arg, struct
unsigned char ovfl_notify;
if (unlikely(buf == NULL || arg == NULL|| regs == NULL || task == NULL)) {
- DPRINT(("[%d] invalid arguments buf=%p arg=%p\n", task->pid, buf, arg));
+ DPRINT(("[%d] invalid arguments buf=%p arg=%p\n", task? task->pid : -1, buf, arg));
return -EINVAL;
}
--
2.17.1