diff mbox series

[ovs-dev,ovs-dev,v1] ipf: avoid accessing to a freed rp.

Message ID 20201014020120.50440-1-hepeng.0320@bytedance.com
State Superseded
Headers show
Series [ovs-dev,ovs-dev,v1] ipf: avoid accessing to a freed rp. | expand

Commit Message

.贺鹏 Oct. 14, 2020, 2:01 a.m. UTC 
if there are multiple pkts in the batch, the loop will access a
freed rp, which cause ovs crash.
---
 lib/ipf.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

0-day Robot Oct. 14, 2020, 3:13 a.m. UTC | #1 
Bleep bloop.  Greetings hepeng.0320, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
ERROR: Author hepeng.0320 <hepeng.0320@bytedance.com> needs to sign off.
Lines checked: 36, Warnings: 0, Errors: 1


Please check this out.  If you feel there has been an error, please email aconole@redhat.com

Thanks,
0-day Robot
.贺鹏 Nov. 17, 2020, 6:52 a.m. UTC | #2 
ping

于2020年10月14日星期三 11:15 <hepeng.0320@bytedance.com> 写道:

if there are multiple pkts in the batch, the loop will access a freed rp,
which cause ovs crash. --- lib/ipf.c | 3 ++- 1 file changed, 2
insertions(+), 1 deletion(-) diff --git a/lib/ipf.c b/lib/ipf.c index
446e89d13..c20bcc0b3 100644 --- a/lib/ipf.c +++ b/lib/ipf.c @@ -1153,7
+1153,7 @@ ipf_post_execute_reass_pkts(struct ipf *ipf, /* Inner batch loop
is constant time since batch size is <= * NETDEV_MAX_BURST. */
DP_PACKET_BATCH_REFILL_FOR_EACH (pb_idx, pb_cnt, pkt, pb) { - if (pkt ==
rp->list->reass_execute_ctx) { + if (rp && pkt ==
rp->list->reass_execute_ctx) { for (int i = 0; i <=
rp->list->last_inuse_idx; i++) { rp->list->frag_list[i].pkt->md.ct_label =
pkt->md.ct_label; rp->list->frag_list[i].pkt->md.ct_mark = pkt->md.ct_mark;
@@ -1206,6 +1206,7 @@ ipf_post_execute_reass_pkts(struct ipf *ipf,
ipf_reassembled_list_remove(rp); dp_packet_delete(rp->pkt); free(rp); + rp
= NULL; } else { dp_packet_batch_refill(pb, pkt, pb_idx); } -- 2.20.1
Ilya Maximets Dec. 21, 2020, 6:33 p.m. UTC | #3 
On 10/14/20 4:01 AM, hepeng.0320 wrote:
> if there are multiple pkts in the batch, the loop will access a
> freed rp, which cause ovs crash.

Fixes: 4ea96698f667 ("Userspace datapath: Add fragmentation handling.")

> ---
>  lib/ipf.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/ipf.c b/lib/ipf.c
> index 446e89d13..c20bcc0b3 100644
> --- a/lib/ipf.c
> +++ b/lib/ipf.c
> @@ -1153,7 +1153,7 @@ ipf_post_execute_reass_pkts(struct ipf *ipf,
>          /* Inner batch loop is constant time since batch size is <=
>           * NETDEV_MAX_BURST. */
>          DP_PACKET_BATCH_REFILL_FOR_EACH (pb_idx, pb_cnt, pkt, pb) {
> -            if (pkt == rp->list->reass_execute_ctx) {
> +            if (rp && pkt == rp->list->reass_execute_ctx) {
>                  for (int i = 0; i <= rp->list->last_inuse_idx; i++) {
>                      rp->list->frag_list[i].pkt->md.ct_label = pkt->md.ct_label;
>                      rp->list->frag_list[i].pkt->md.ct_mark = pkt->md.ct_mark;
> @@ -1206,6 +1206,7 @@ ipf_post_execute_reass_pkts(struct ipf *ipf,
>                  ipf_reassembled_list_remove(rp);
>                  dp_packet_delete(rp->pkt);
>                  free(rp);
> +                rp = NULL;
>              } else {
>                  dp_packet_batch_refill(pb, pkt, pb_idx);
>              }
> 

Hi.  The patch looks correct to me.

Could you, please, provide a 'Signed-off-by' tag so we could accept it?
I see that you have them in your other patches, but not in this one.

Best regards, Ilya Maximets.
diff mbox series

Patch

diff --git a/lib/ipf.c b/lib/ipf.c
index 446e89d13..c20bcc0b3 100644
--- a/lib/ipf.c
+++ b/lib/ipf.c
@@ -1153,7 +1153,7 @@  ipf_post_execute_reass_pkts(struct ipf *ipf,
         /* Inner batch loop is constant time since batch size is <=
          * NETDEV_MAX_BURST. */
         DP_PACKET_BATCH_REFILL_FOR_EACH (pb_idx, pb_cnt, pkt, pb) {
-            if (pkt == rp->list->reass_execute_ctx) {
+            if (rp && pkt == rp->list->reass_execute_ctx) {
                 for (int i = 0; i <= rp->list->last_inuse_idx; i++) {
                     rp->list->frag_list[i].pkt->md.ct_label = pkt->md.ct_label;
                     rp->list->frag_list[i].pkt->md.ct_mark = pkt->md.ct_mark;
@@ -1206,6 +1206,7 @@  ipf_post_execute_reass_pkts(struct ipf *ipf,
                 ipf_reassembled_list_remove(rp);
                 dp_packet_delete(rp->pkt);
                 free(rp);
+                rp = NULL;
             } else {
                 dp_packet_batch_refill(pb, pkt, pb_idx);
             }