ddb: annotate some commands with DB_CMD_MEMSAFE

[mhorne]

Summary

This is not completely exhaustive, but covers a large majority of
commands in the tree.

[markj]

I think this is ok, but IMHO the approach of trying to tag every MEMSAFE command as such is probably not useful. In practice there's only a few basic commands that one will want to run on a customer system, and if that's not enough, then the restrictions of MEMSAFE will probably mean that DDB is a dead end for diagnosing the problem at hand. At least, I would omit its use in drivers (ciss, lapic, dmar, etc.). I don't object to this patch, it's just a comment.

sys/ddb/db_watch.c

219

Is it useful to be able to list watchpoints when one can't create them?

sys/dev/aic7xxx/aic79xx_osm.c

1527 ↗

(On Diff #107308)

I would not bother modifying this driver at all.

sys/i386/i386/machdep.c

725

This is not MEMSAFE.

sys/kern/subr_pcpu.c

402

This command should verify that id is valid.

sys/kern/subr_turnstile.c

1266

It's kind of weird that alises have to be flagged as well. Is it not easy to fix that?

sys/vm/vm_object.c

2849

Not asking you to change anything, but I have a hard time believing this command is useful to anyone, ever. :)

sys/x86/iommu/intel_drv.c

1323 ↗

(On Diff #107308)

This addr parameter is used as an index, and there's no bounds checks. I'd be inclined to omit the MEMSAFE flag here.

[mhorne]

In D35583#807077, @markj wrote:

I think this is ok, but IMHO the approach of trying to tag every MEMSAFE command as such is probably not useful. In practice there's only a few basic commands that one will want to run on a customer system, and if that's not enough, then the restrictions of MEMSAFE will probably mean that DDB is a dead end for diagnosing the problem at hand. At least, I would omit its use in drivers (ciss, lapic, dmar, etc.). I don't object to this patch, it's just a comment.

You are right. I made the changes somewhat mechanically, so I was thinking more about being complete than the practicality of each command. I do think we can pare down the diff a bit.

sys/ddb/db_watch.c

219

Well, I do enable the watchpoint commands in D35584. I do not think there is a problem in allowing that? (yes, maybe not so useful for the described use-case)

sys/kern/subr_pcpu.c

402

Yes it should. I'll add a separate commit that checks id >= 0 && id <= mp_maxid.

sys/kern/subr_turnstile.c

1266

Unfortunately not without changing the way aliases are created. At present they are registered as entirely separate command structures that execute the same function.

[mhorne]

Handle feedback from markj, remove one unsafe command and several unnecessary ones.

[jhb]

I haven't looked at all of these in detail, but I think the general idea is ok.