Re: [PATCH v4 17/38] scsi: sg: convert mmap hook to mmap_prepare and rework

[Suren Baghdasaryan]

On Sat, Oct 3, 2026 at 6:39 PM Lorenzo Stoakes (ARM) <ljs@xxxxxxxxxx> wrote:
>
> Move from the deprecated mmap hook to the new mmap_prepare hook.
>
> We are mapping kernel pages here, so use the discontiguous kernel mapping
> mmap action to do so.
>
> Unwind the rather confusing loop and instead map as many pages as we can at
> one time.
>
> Note that we do not need to pay attention to rsv_schp->k_use_sg here, as
> the pages are populated for the length of the buffer at
> rsv_schp->page_order granularity as compound pages.
>
> The discontiguous kernel page mapping logic handles the compound pages for
> us.
>
> sfp->mmap_called keeps the buffer stable for us. As before it is never
> cleared, so a failed mmap also leaves it set.
>
> We also remove some useless vma, vma->vm_file NULL checks - these will
> always be non-NULL if you reached the mmap hook logic.
>
> We retain log output for consistency, but change what's output on page
> mapping to indicate that sg_discontig_get() does the work now.
>
> Note that we drop the VMA_IO_BIT flag for the VMA here. It was never
> necessary as we invoke alloc_pages() which gives us refcounted folios that
> are fine for GUP to access (VMA_IO_BIT would prevent that).
>
> Signed-off-by: Lorenzo Stoakes (ARM) <ljs@xxxxxxxxxx>

The code looks correct but I must disclaim that I have no experience
with this driver.

Reviewed-by: Suren Baghdasaryan <surenb@xxxxxxxxxx>


> ---
>  drivers/scsi/sg.c | 115 ++++++++++++++++++++++++------------------------------
>  1 file changed, 51 insertions(+), 64 deletions(-)
>
> diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
> index 5408f002e6c0..3f9e08725602 100644
> --- a/drivers/scsi/sg.c
> +++ b/drivers/scsi/sg.c
> @@ -1212,85 +1212,72 @@ sg_fasync(int fd, struct file *filp, int mode)
>         return fasync_helper(fd, filp, mode, &sfp->async_qp);
>  }
>
> -static vm_fault_t
> -sg_vma_fault(struct vm_fault *vmf)
> +static int sg_discontig_init(void *vm_private_data, void **private)
>  {
> -       struct vm_area_struct *vma = vmf->vma;
> -       Sg_fd *sfp;
> -       unsigned long offset, len, sa;
> -       Sg_scatter_hold *rsv_schp;
> -       int k, length;
> -
> -       if ((NULL == vma) || (!(sfp = (Sg_fd *) vma->vm_private_data)))
> -               return VM_FAULT_SIGBUS;
> -       rsv_schp = &sfp->reserve;
> -       offset = vmf->pgoff << PAGE_SHIFT;
> -       if (offset >= rsv_schp->bufflen)
> -               return VM_FAULT_SIGBUS;
> -       SCSI_LOG_TIMEOUT(3, sg_printk(KERN_INFO, sfp->parentdp,
> -                                     "sg_vma_fault: offset=%lu, scatg=%d\n",
> -                                     offset, rsv_schp->k_use_sg));
> -       sa = vma->vm_start;
> -       length = 1 << (PAGE_SHIFT + rsv_schp->page_order);
> -       for (k = 0; k < rsv_schp->k_use_sg && sa < vma->vm_end; k++) {
> -               len = vma->vm_end - sa;
> -               len = (len < length) ? len : length;
> -               if (offset < len) {
> -                       struct page *page = rsv_schp->pages[k] + (offset >> PAGE_SHIFT);
> -                       get_page(page); /* increment page count */
> -                       vmf->page = page;
> -                       return 0; /* success */
> -               }
> -               sa += len;
> -               offset -= len;
> +       const unsigned long req_sz = (unsigned long)*private;
> +       Sg_fd *sfp = vm_private_data;
> +       Sg_scatter_hold *rsv_schp = &sfp->reserve;
> +       int err = 0;
> +
> +       mutex_lock(&sfp->f_mutex);
> +       if (req_sz > rsv_schp->bufflen) {
> +               err = -ENOMEM;  /* cannot map more than reserved buffer */
> +               goto out;
> +       }
> +       sfp->mmap_called = 1; /* Prevents changes to buffer size. */
> +out:
> +       mutex_unlock(&sfp->f_mutex);
> +       return err;
> +}
> +
> +static int
> +sg_discontig_get(struct discontig_kernel_page_state *state)
> +{
> +       Sg_fd *sfp = state->vm_private_data;
> +       Sg_scatter_hold *rsv_schp = &sfp->reserve;
> +       const unsigned int order = rsv_schp->page_order;
> +       const pgoff_t nr_pages = state->nr_pages_mapped;
> +
> +       if (nr_pages >= (rsv_schp->bufflen >> PAGE_SHIFT)) {
> +               discontig_kernel_map_abort(state);
> +               return 0;
>         }
>
> -       return VM_FAULT_SIGBUS;
> +       SCSI_LOG_TIMEOUT(3, sg_printk(KERN_INFO, sfp->parentdp,
> +                                     "%s: offset=%lu, scatg=%d\n", __func__,
> +                                     nr_pages << PAGE_SHIFT, rsv_schp->k_use_sg));
> +
> +       discontig_kernel_map_page(state, rsv_schp->pages[nr_pages >> order]);
> +       return 0;
>  }
>
> -static const struct vm_operations_struct sg_mmap_vm_ops = {
> -       .fault = sg_vma_fault,
> +static const struct discontig_kernel_page_ops sg_discontig_ops = {
> +       .init = sg_discontig_init,
> +       .get = sg_discontig_get,
>  };
>
>  static int
> -sg_mmap(struct file *filp, struct vm_area_struct *vma)
> +sg_mmap_prepare(struct vm_area_desc *desc)
>  {
> -       Sg_fd *sfp;
> -       unsigned long req_sz, len, sa;
> -       Sg_scatter_hold *rsv_schp;
> -       int k, length;
> -       int ret = 0;
> +       Sg_fd *sfp = desc->file->private_data;
> +       const unsigned long req_sz = vma_desc_size(desc);
>
> -       if ((!filp) || (!vma) || (!(sfp = (Sg_fd *) filp->private_data)))
> +       if (!sfp)
>                 return -ENXIO;
> -       req_sz = vma->vm_end - vma->vm_start;
> +
>         SCSI_LOG_TIMEOUT(3, sg_printk(KERN_INFO, sfp->parentdp,
>                                       "sg_mmap starting, vm_start=%p, len=%d\n",
> -                                     (void *) vma->vm_start, (int) req_sz));
> -       if (vma->vm_pgoff)
> +                                     (void *) desc->start, (int) req_sz));
> +
> +       if (desc->pgoff)
>                 return -EINVAL; /* want no offset */
> -       rsv_schp = &sfp->reserve;
> -       mutex_lock(&sfp->f_mutex);
> -       if (req_sz > rsv_schp->bufflen) {
> -               ret = -ENOMEM;  /* cannot map more than reserved buffer */
> -               goto out;
> -       }
>
> -       sa = vma->vm_start;
> -       length = 1 << (PAGE_SHIFT + rsv_schp->page_order);
> -       for (k = 0; k < rsv_schp->k_use_sg && sa < vma->vm_end; k++) {
> -               len = vma->vm_end - sa;
> -               len = (len < length) ? len : length;
> -               sa += len;
> -       }
> +       vma_desc_set_flags(desc, VMA_DONTEXPAND_BIT, VMA_DONTDUMP_BIT);
> +       desc->private_data = sfp;
>
> -       sfp->mmap_called = 1;
> -       vm_flags_set(vma, VM_IO | VM_DONTEXPAND | VM_DONTDUMP);
> -       vma->vm_private_data = sfp;
> -       vma->vm_ops = &sg_mmap_vm_ops;
> -out:
> -       mutex_unlock(&sfp->f_mutex);
> -       return ret;
> +       mmap_action_map_discontig_kernel_pages(desc, (void *)req_sz,
> +                                              &sg_discontig_ops);
> +       return 0;
>  }
>
>  static void
> @@ -1415,7 +1402,7 @@ static const struct file_operations sg_fops = {
>         .unlocked_ioctl = sg_ioctl,
>         .compat_ioctl = compat_ptr_ioctl,
>         .open = sg_open,
> -       .mmap = sg_mmap,
> +       .mmap_prepare = sg_mmap_prepare,
>         .release = sg_release,
>         .fasync = sg_fasync,
>  };
>
> --
> 2.55.0
>