On Thu, 2026-07-23 at 15:30 +0000, [email protected] wrote: > 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 > > + > > [ ... ] Thanks for the review.
The module-build observation is correct. scsi_scan_host_selected() is an internal SCSI midlayer helper and is not exported for use by a modular storvsc driver. The successful built-in kernel build did not expose that problem so I never caught it. The callback should instead use the exported scsi_scan_target() interface and should not include scsi_priv.h. I am also reviewing the FC error-handler concern before posting a revised patch. Devices discovered directly below the SCSI host do not have an fc_rport parent, while the storvsc timeout path delegates to fc_eh_timed_out(). A revised patch must ensure that this topology cannot lead to a NULL rport dereference. I will address both points in v2. Thanks, Laurence
