Page MenuHomeFreeBSD

cam: coccinelle generated fix
AbandonedPublic

Authored by imp on May 20 2021, 5:05 AM.

Details

Reviewers
slm
Group Reviewers
cam

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 39317
Build 36206: arc lint + arc unit

Event Timeline

imp requested review of this revision.May 20 2021, 5:05 AM

Current coccinelle patch

@@
type T;
identifier V;
expression PATH;
expression PRIO;
@@

T V;
...
- memset(&V, 0, sizeof(V));
...
- xpt_setup_ccb(&V.ccb_h, PATH, PRIO)
+ xpt_setup_stack_ccb(&V.ccb_h, sizeof(V), PATH, PRIO)
@@
type T;
identifier V;
expression PATH;
expression PRIO;
@@

T V;
...
- bzero(&V, sizeof(V));
...
- xpt_setup_ccb(&V.ccb_h, PATH, PRIO)
+ xpt_setup_stack_ccb(&V.ccb_h, sizeof(V), PATH, PRIO)
@@
type T;
identifier V;
expression PATH;
expression PRIO;
@@

T V;
...
- xpt_setup_ccb(&V.ccb_h, PATH, PRIO)
+ xpt_setup_stack_ccb(&V.ccb_h, sizeof(V), PATH, PRIO)

better script, but still misses a few things

Oh, this works better

// Script to find all the uses of ccb's allocated on the stack and change
// them to use the new setup function which bzeros before doing a normal
// setup of the CCB.
@@
identifier V;
expression PATH;
expression PRIO;
@@

- memset(&V, 0, sizeof(V));
...
- xpt_setup_ccb(&V.ccb_h, PATH, PRIO)
+ xpt_setup_stack_ccb(&V.ccb_h, sizeof(V), PATH, PRIO)
@@
identifier V;
expression PATH;
expression PRIO;
@@

- bzero(&V, sizeof(V));
...
- xpt_setup_ccb(&V.ccb_h, PATH, PRIO)
+ xpt_setup_stack_ccb(&V.ccb_h, sizeof(V), PATH, PRIO)
@@
identifier V;
expression PATH;
expression PRIO;
@@

- xpt_setup_ccb(&V.ccb_h, PATH, PRIO)
+ xpt_setup_stack_ccb(&V.ccb_h, sizeof(V), PATH, PRIO)

updated the patch based on this

Found five errors by hand at least, but unsure how to automate it.

sys/cam/ata/ata_da.c
1121–1122

this is a bug, this should be deleted.

sys/cam/cam_xpt.c
502

this is a bug, but we dump the path into into this ccb, and then dump the path into it, then setup things, which is a pattern not otherwise done.

sys/cam/scsi/scsi_target.c
275

also an ordering bug

sys/dev/sym/sym_hipd.c
3428

ordering bug.

3449–3450

hmmm, this one is a bug...

Tweaked the script to replace the bzero/memset with the new function.

// Script to find all the uses of ccb's allocated on the stack and change
// them to use the new setup function which bzeros before doing a normal
// setup of the CCB.
@@
identifier V;
expression PATH;
expression PRIO;
@@

- memset(&V, 0, sizeof(V));
+ xpt_setup_stack_ccb(&V.ccb_h, sizeof(V), PATH, PRIO);
...
- xpt_setup_ccb(&V.ccb_h, PATH, PRIO);
@@
identifier V;
expression PATH;
expression PRIO;
@@

- bzero(&V, sizeof(V));
+ xpt_setup_stack_ccb(&V.ccb_h, sizeof(V), PATH, PRIO);
...
- xpt_setup_ccb(&V.ccb_h, PATH, PRIO);
@@
identifier V;
expression PATH;
expression PRIO;
@@

- xpt_setup_ccb(&V.ccb_h, PATH, PRIO);
+ xpt_setup_stack_ccb(&V.ccb_h, sizeof(V), PATH, PRIO);

Regen with newer coccinelle patch

The latest script still has 2 bugs: ata_da.c (extra memset) and cam_xpt.c (need to create a temp path like is done elsewhere but aren't)

run through git clang-format

mostly better enough to prefer this.

How does cocinelle know the CCB is from the stack, as opposed to being allocated dynamically?

How does cocinelle know the CCB is from the stack, as opposed to being allocated dynamically?

Because the script uses a variable, not a pointer, to do the matching. This can distinguish between
a pointer that's cast about vs one that's on the stack. My script is a bit primitive, though, in that
it doesn't find xpt_setup_ccb((struct ccb_hdr *)&stack), but I couldn't find any of those in the tree.
Coiccinelle has a fairly sophisticated semantic matching algorithm. It's main use is migrating APIs
in the linux kernel, which is why the output is linux kernel normal form...

Free the path correctly