Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues: - [Low] The patch calls the unexported internal midlayer function `scsi_scan_host_selected()` and includes the private header `"scsi_priv.h"` from a hardware driver, which will break module builds. Pre-existing issues: - [High] Timeouts on targets scanned without an `fc_rport` parent will cause a NULL pointer dereference in the FC transport error handler. -- --- Patch [1]: [PATCH] scsi: storvsc: Support manual scans for all Hyper-V targets --- commit ce267d7e85b7a51757a8afe1188939d3c5146d27 Author: Laurence Oberman <[email protected]> scsi: storvsc: Support manual scans for all Hyper-V targets This patch provides a storvsc-specific user_scan callback that directly invokes scsi_scan_host_selected(). This bypasses the single-rport lookup in fc_user_scan() to allow userspace to explicitly scan any target supported by storvsc. The goal is to avoid requiring storvsc to create and manage synthetic fc_rports for every Hyper-V target. > diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c > index f03dc03a42c35..eb40473a8aad6 100644 > --- a/drivers/scsi/storvsc_drv.c > +++ b/drivers/scsi/storvsc_drv.c > @@ -43,6 +43,7 @@ > #include <scsi/scsi_dbg.h> > #include <scsi/scsi_transport_fc.h> > #include <scsi/scsi_transport.h> > +#include "scsi_priv.h" [Severity: Low] Will this cause an architectural layering violation? Including a private midlayer header like scsi_priv.h from a low-level hardware driver normally breaks subsystem encapsulation. [ ... ] > @@ -550,6 +551,17 @@ static void storvsc_host_scan(struct work_struct *work) > scsi_scan_host(host); > } > > +#if IS_ENABLED(CONFIG_SCSI_FC_ATTRS) > +static int storvsc_user_scan(struct Scsi_Host *host, > + unsigned int channel, > + unsigned int id, > + u64 lun) > +{ > + return scsi_scan_host_selected(host, channel, id, lun, > + SCSI_SCAN_MANUAL); > +} [Severity: Low] Does this cause a module build failure during the modpost link stage? The function scsi_scan_host_selected() in drivers/scsi/scsi_scan.c lacks an EXPORT_SYMBOL directive. If the kernel is configured to build storvsc as a module (CONFIG_HYPERV_STORAGE=m), it seems it will fail to link. [Severity: High] This is a pre-existing issue, but does creating a topology without an fc_rport parent lead to a NULL pointer dereference during error handling? If a SCSI command times out on a target scanned directly under the SCSI host (which this manual scan facilitates), storvsc_eh_timed_out() delegates to the FC transport's fc_eh_timed_out(). Because the target was scanned under the host device instead of an fc_rport, starget_to_rport() evaluates to NULL. This NULL pointer is then unconditionally dereferenced in drivers/scsi/scsi_transport_fc.c:fc_eh_timed_out(): fc_eh_timed_out() { ... struct fc_rport *rport = starget_to_rport(scsi_target(scmd->device)); if (rport->port_state == FC_PORTSTATE_BLOCKED) ... } Can timeouts under hardware or network load on these targets trigger a kernel panic? > +#endif > + [ ... ] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
