Re: [RFC PATCH v3 05/10] mm/mshare: Add ioctl support

[Anthony Yznaga]

On 10/14/24 1:08 PM, Jann Horn wrote:
> On Wed, Sep 4, 2024 at 1:22 AM Anthony Yznaga <anthony.yznaga@xxxxxxxxxx> wrote:
> > Reserve a range of ioctls for msharefs and add the first two ioctls
> > to get and set the start address and size of an mshare region.
> [...]
> > +static long
> > +msharefs_set_size(struct mm_struct *mm, struct mshare_data *m_data,
> > +                       struct mshare_info *minfo)
> > +{
> > +       unsigned long end = minfo->start + minfo->size;
> > +
> > +       /*
> > +        * Validate alignment for start address, and size
> > +        */
> > +       if ((minfo->start | end) & (PGDIR_SIZE - 1)) {
> > +               spin_unlock(&m_data->m_lock);
> > +               return -EINVAL;
> > +       }
> > +
> > +       mm->mmap_base = minfo->start;
> > +       mm->task_size = minfo->size;
> > +       if (!mm->task_size)
> > +               mm->task_size--;
> > +
> > +       m_data->minfo.start = mm->mmap_base;
> > +       m_data->minfo.size = mm->task_size;
> > +       spin_unlock(&m_data->m_lock);
> > +
> > +       return 0;
> > +}
> > +
> > +static long
> > +msharefs_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> > +{
> > +       struct mshare_data *m_data = filp->private_data;
> > +       struct mm_struct *mm = m_data->mm;
> > +       struct mshare_info minfo;
> > +
> > +       switch (cmd) {
> > +       case MSHAREFS_GET_SIZE:
> > +               spin_lock(&m_data->m_lock);
> > +               minfo = m_data->minfo;
> > +               spin_unlock(&m_data->m_lock);
> > +
> > +               if (copy_to_user((void __user *)arg, &minfo, sizeof(minfo)))
> > +                       return -EFAULT;
> > +
> > +               return 0;
> > +
> > +       case MSHAREFS_SET_SIZE:
> > +               if (copy_from_user(&minfo, (struct mshare_info __user *)arg,
> > +                       sizeof(minfo)))
> > +                       return -EFAULT;
> > +
> > +               /*
> > +                * If this mshare region has been set up once already, bail out
> > +                */
> > +               spin_lock(&m_data->m_lock);
> > +               if (m_data->minfo.start != 0) {
> Is there actually anything that prevents msharefs_set_size() from
> setting up m_data with ->minfo.start==0, so that a second
> MSHAREFS_SET_SIZE invocation will succeed? It would probably be more
> reliable to have a separate flag for "has this thing been set up yet".

Thanks for pointing this out. Yes, this is problematic. A start address 
of 0 generally won't work because mmap() will fail unless there are 
sufficient privileges (cap_map_addr will return -EPERM). I already have 
changes to use the size to indicate initialization, but it may make 
sense to have flags.


Anthony

> > +                       spin_unlock(&m_data->m_lock);
> > +                       return -EINVAL;
> > +               }
> > +
> > +               return msharefs_set_size(mm, m_data, &minfo);
> > +
> > +       default:
> > +               return -ENOTTY;
> > +       }
> > +}