Can vm_mmap()/vm_map_remove() be called with giant held?
(linuxolator dvb patches)
Juergen Lock
nox at jelal.kn-bremen.de
Sat Jan 29 23:56:13 UTC 2011
On Sat, Jan 29, 2011 at 10:51:05PM +0200, Kostik Belousov wrote:
> On Sat, Jan 29, 2011 at 09:10:00PM +0100, Juergen Lock wrote:
> > Hi!
> >
> > I was kinda hoping to be able to post a correct patch in public but
> > getting an answer to ${Subject} seems to be more difficult than I
> > thought... :) So, does anyone here know? copyout_map() and
> You do not need Giant locked for vm_map* functions.
>
The question was more do I need to drop it first before calling them...
> > copyout_unmap() are copied from ksyms_map() from sys/dev/ksyms/ksyms.c
> > - should there maybe be global versions instead of two static copies
> > each, and what would be good names? And giant is taken by linux_ioctl()
> Would you make a patch for this ?
>
Heh if you want me to... Where should they go and are my name choices ok?
> > in the same source file before calling the parts I added. So here
> > comes the patch, it is to add support for dvb ioctls to the linuxolator
> > as discussed on -emulation earlier in this thread:
> >
> > http://lists.freebsd.org/pipermail/freebsd-multimedia/2011-January/011575.html
> >
> > (patch also at:
> >
> > http://people.freebsd.org/~nox/dvb/linux-dvb.patch
> >
> > and a version for 8, which is what I tested with w_scan on dvb-s2
> > and dvb-t, and Andrew Gallatin also tested it with SageTV:
> >
> > http://people.freebsd.org/~nox/dvb/linux-dvb-8.patch
> >
> > )
> >
> > Thanx!
> > Juergen
> >
> > Index: src/sys/compat/linux/linux_ioctl.c
> > ===================================================================
> > RCS file: /home/scvs/src/sys/compat/linux/linux_ioctl.c,v
> > retrieving revision 1.167
> > diff -u -p -r1.167 linux_ioctl.c
> > --- src/sys/compat/linux/linux_ioctl.c 30 Dec 2010 02:18:04 -0000 1.167
> > +++ src/sys/compat/linux/linux_ioctl.c 18 Jan 2011 17:10:21 -0000
> > @@ -59,6 +59,14 @@ __FBSDID("$FreeBSD: src/sys/compat/linux
> > #include <sys/sx.h>
> > #include <sys/tty.h>
> > #include <sys/uio.h>
> > +#include <sys/types.h>
> > +#include <sys/mman.h>
> > +#include <sys/resourcevar.h>
> > +
> > +#include <vm/vm.h>
> > +#include <vm/pmap.h>
> > +#include <vm/vm_extern.h>
> > +#include <vm/vm_map.h>
> >
> > #include <net/if.h>
> > #include <net/if_dl.h>
> > @@ -83,6 +91,9 @@ __FBSDID("$FreeBSD: src/sys/compat/linux
> > #include <compat/linux/linux_videodev.h>
> > #include <compat/linux/linux_videodev_compat.h>
> >
> > +#include <compat/linux/linux_dvb.h>
> > +#include <compat/linux/linux_dvb_compat.h>
> > +
> > CTASSERT(LINUX_IFNAMSIZ == IFNAMSIZ);
> >
> > static linux_ioctl_function_t linux_ioctl_cdrom;
> > @@ -97,6 +108,7 @@ static linux_ioctl_function_t linux_ioct
> > static linux_ioctl_function_t linux_ioctl_drm;
> > static linux_ioctl_function_t linux_ioctl_sg;
> > static linux_ioctl_function_t linux_ioctl_v4l;
> > +static linux_ioctl_function_t linux_ioctl_dvb;
> > static linux_ioctl_function_t linux_ioctl_special;
> > static linux_ioctl_function_t linux_ioctl_fbsd_usb;
> >
> > @@ -124,6 +136,8 @@ static struct linux_ioctl_handler sg_han
> > { linux_ioctl_sg, LINUX_IOCTL_SG_MIN, LINUX_IOCTL_SG_MAX };
> > static struct linux_ioctl_handler video_handler =
> > { linux_ioctl_v4l, LINUX_IOCTL_VIDEO_MIN, LINUX_IOCTL_VIDEO_MAX };
> > +static struct linux_ioctl_handler dvb_handler =
> > +{ linux_ioctl_dvb, LINUX_IOCTL_DVB_MIN, LINUX_IOCTL_DVB_MAX };
> > static struct linux_ioctl_handler fbsd_usb =
> > { linux_ioctl_fbsd_usb, FBSD_LUSB_MIN, FBSD_LUSB_MAX };
> >
> > @@ -139,6 +153,7 @@ DATA_SET(linux_ioctl_handler_set, privat
> > DATA_SET(linux_ioctl_handler_set, drm_handler);
> > DATA_SET(linux_ioctl_handler_set, sg_handler);
> > DATA_SET(linux_ioctl_handler_set, video_handler);
> > +DATA_SET(linux_ioctl_handler_set, dvb_handler);
> > DATA_SET(linux_ioctl_handler_set, fbsd_usb);
> >
> > struct handler_element
> > @@ -2989,6 +3004,251 @@ linux_ioctl_special(struct thread *td, s
> > }
> >
> > /*
> > + * Map some anonymous memory in user space of size sz, rounded up to the page
> > + * boundary.
> > + */
> > +static int
> > +copyout_map(struct thread *td, vm_offset_t *addr, size_t sz)
> > +{
> > + struct vmspace *vms = td->td_proc->p_vmspace;
> Style recommends not to initialize in the declaration section.
> [I do not repeat systematic style violations notes below].
>
Heh as I said copyout_map() and copyout_unmap() are exact copies
from ksyms_map() out of sys/dev/ksyms/ksyms.c so I didnt change
those other than the names...
> > + int error;
> > + vm_size_t size;
> > +
> > +
> One empty line is enough.
>
> > + /*
> > + * Map somewhere after heap in process memory.
> > + */
> > + PROC_LOCK(td->td_proc);
> > + *addr = round_page((vm_offset_t)vms->vm_daddr +
> > + lim_max(td->td_proc, RLIMIT_DATA));
> > + PROC_UNLOCK(td->td_proc);
> Are you sure that this is needed ? Why not leave the address selection
> to the VM ?
>
I don't know, maybe sys/dev/ksyms/ksyms.c has a reason?
> > +
> > + /* round size up to page boundry */
> > + size = (vm_size_t) round_page(sz);
> > +
> > + error = vm_mmap(&vms->vm_map, addr, size, PROT_READ | PROT_WRITE,
> > + VM_PROT_ALL, MAP_PRIVATE | MAP_ANON, OBJT_DEFAULT, NULL, 0);
> > +
> > + return (error);
> > +}
> > +
> > +/*
> > + * Unmap memory in user space.
> > + */
> > +static int
> > +copyout_unmap(struct thread *td, vm_offset_t addr, size_t sz)
> > +{
> > + vm_map_t map;
> > + vm_size_t size;
> > +
> > + map = &td->td_proc->p_vmspace->vm_map;
> > + size = (vm_size_t) round_page(sz);
> > +
> > + if (!vm_map_remove(map, addr, addr + size))
> Better do if (vm_map_remove(...) != KERN_SUCCESS)
>
> > + return (EINVAL);
> > +
> > + return (0);
> > +}
> > +
> > +static int
> > +linux_to_bsd_dtv_properties(struct l_dtv_properties *lvps, struct dtv_properties *vps)
> > +{
> Empty line between (empty) declaration section and executable statements.
>
Ok.
> > + vps->num = lvps->num;
> > + vps->props = PTRIN(lvps->props); /* possible pointer size conversion */
> > + return (0);
> > +}
> > +
> > +static int
> > +linux_to_bsd_dtv_property(struct l_dtv_property *lvp, struct dtv_property *vp)
> > +{
> > + /*
> > + * everything until u.buffer.reserved2 is fixed size so
> > + * just memcpy it.
> > + */
> > + memcpy(vp, lvp, offsetof(struct l_dtv_property, u.buffer.reserved2));
> > + /*
> > + * the pointer may be garbage since it's part of a union,
> > + * currently no Linux code uses it so just set it to NULL.
> > + */
> > + vp->u.buffer.reserved2 = NULL;
> > + vp->result = lvp->result;
> > + return (0);
> > +}
> > +
> > +static int
> > +bsd_to_linux_dtv_property(struct dtv_property *vp, struct l_dtv_property *lvp)
> > +{
> > + /*
> > + * everything until u.buffer.reserved2 is fixed size so
> > + * just memcpy it.
> > + */
> > + memcpy(lvp, vp, offsetof(struct l_dtv_property, u.buffer.reserved2));
> > + /*
> > + * the pointer may be garbage since it's part of a union,
> > + * currently no Linux code uses it so just set it to NULL.
> > + */
> > + lvp->u.buffer.reserved2 = PTROUT(NULL);
> > + lvp->result = vp->result;
> > + return (0);
> > +}
> > +
> > +static int
> > +linux_ioctl_dvb(struct thread *td, struct linux_ioctl_args *args)
> > +{
> > + struct file *fp;
> > + int error, i;
> > + struct l_dtv_properties l_vps;
> > + struct dtv_properties vps;
> > + struct l_dtv_property *l_vp = NULL, *l_p;
> > + struct dtv_property *vp = NULL, *p;
> > + size_t l_propsiz, propsiz;
> > + vm_offset_t uvp;
> > +
> > + switch (args->cmd & 0xffff) {
> > + case LINUX_AUDIO_STOP:
> > + case LINUX_AUDIO_PLAY:
> > + case LINUX_AUDIO_PAUSE:
> > + case LINUX_AUDIO_CONTINUE:
> > + case LINUX_AUDIO_SELECT_SOURCE:
> > + case LINUX_AUDIO_SET_MUTE:
> > + case LINUX_AUDIO_SET_AV_SYNC:
> > + case LINUX_AUDIO_SET_BYPASS_MODE:
> > + case LINUX_AUDIO_CHANNEL_SELECT:
> > + case LINUX_AUDIO_CLEAR_BUFFER:
> > + case LINUX_AUDIO_SET_ID:
> > + case LINUX_AUDIO_SET_STREAMTYPE:
> > + case LINUX_AUDIO_SET_EXT_ID:
> > + case LINUX_AUDIO_BILINGUAL_CHANNEL_SELECT:
> > + case LINUX_DMX_START:
> > + case LINUX_DMX_STOP:
> > + case LINUX_DMX_SET_BUFFER_SIZE:
> > + case LINUX_NET_REMOVE_IF:
> > + case LINUX_FE_DISEQC_RESET_OVERLOAD:
> > + case LINUX_FE_DISEQC_SEND_BURST:
> > + case LINUX_FE_SET_TONE:
> > + case LINUX_FE_SET_VOLTAGE:
> > + case LINUX_FE_ENABLE_HIGH_LNB_VOLTAGE:
> > + case LINUX_FE_DISHNETWORK_SEND_LEGACY_CMD:
> > + case LINUX_FE_SET_FRONTEND_TUNE_MODE:
> > + case LINUX_CA_RESET:
> > + if ((args->cmd & IOC_DIRMASK) != LINUX_IOC_VOID)
> > + return ENOIOCTL;
> > + args->cmd = (args->cmd & 0xffff) | IOC_VOID;
> > + break;
> > +
> > + case LINUX_DMX_REMOVE_PID:
> > + /* overlaps with LINUX_NET_ADD_IF */
> > + if ((args->cmd & IOC_DIRMASK) == LINUX_IOC_INOUT)
> > + goto net_add_if;
> > + /* FALLTHRU */
> > + case LINUX_AUDIO_SET_MIXER:
> > + case LINUX_AUDIO_SET_ATTRIBUTES:
> > + case LINUX_AUDIO_SET_KARAOKE:
> > + case LINUX_DMX_SET_FILTER:
> > + case LINUX_DMX_SET_PES_FILTER:
> > + case LINUX_DMX_SET_SOURCE:
> > + case LINUX_DMX_ADD_PID:
> > + case LINUX_FE_DISEQC_SEND_MASTER_CMD:
> > + case LINUX_FE_SET_FRONTEND:
> > + case LINUX_CA_SEND_MSG:
> > + case LINUX_CA_SET_DESCR:
> > + case LINUX_CA_SET_PID:
> > + args->cmd = (args->cmd & ~IOC_DIRMASK) | IOC_IN;
> > + break;
> > +
> > + case LINUX_AUDIO_GET_STATUS:
> > + case LINUX_AUDIO_GET_CAPABILITIES:
> > + case LINUX_AUDIO_GET_PTS:
> > + case LINUX_DMX_GET_PES_PIDS:
> > + case LINUX_DMX_GET_CAPS:
> > + case LINUX_FE_GET_INFO:
> > + case LINUX_FE_DISEQC_RECV_SLAVE_REPLY:
> > + case LINUX_FE_READ_STATUS:
> > + case LINUX_FE_READ_BER:
> > + case LINUX_FE_READ_SIGNAL_STRENGTH:
> > + case LINUX_FE_READ_SNR:
> > + case LINUX_FE_READ_UNCORRECTED_BLOCKS:
> > + case LINUX_FE_GET_FRONTEND:
> > + case LINUX_FE_GET_EVENT:
> > + case LINUX_CA_GET_CAP:
> > + case LINUX_CA_GET_SLOT_INFO:
> > + case LINUX_CA_GET_DESCR_INFO:
> > + case LINUX_CA_GET_MSG:
> > + args->cmd = (args->cmd & ~IOC_DIRMASK) | IOC_OUT;
> > + break;
> > +
> > + case LINUX_DMX_GET_STC:
> > + case LINUX_NET_GET_IF:
> > + net_add_if:
> > + args->cmd = (args->cmd & ~IOC_DIRMASK) | IOC_INOUT;
> > + break;
> > +
> > + case LINUX_FE_SET_PROPERTY:
> > + case LINUX_FE_GET_PROPERTY:
> > + error = copyin((void *) args->arg, &l_vps, sizeof(l_vps));
> > + if (error)
> > + return (error);
> > + linux_to_bsd_dtv_properties(&l_vps, &vps);
> > + if ((vps.num == 0) || vps.num > DTV_IOCTL_MAX_MSGS)
> > + return EINVAL;
> > +
> > + l_propsiz = vps.num * sizeof(*l_vp);
> > + propsiz = vps.num * sizeof(*vp);
> > + if (((l_vp = malloc(l_propsiz, M_LINUX, M_WAITOK)) == NULL) ||
> > + (vp = malloc(propsiz, M_LINUX, M_WAITOK)) == NULL) {
> Malloc(M_WAITOK) is guaranteed to never return NULL, you do not need the
> checks.
>
Ok.
> > + error = ENOMEM;
> > + goto out2;
> > + }
> > + error = copyin((void *) vps.props, l_vp, l_propsiz);
> > + if (error)
> > + goto out2;
> > + for (i = vps.num, l_p = l_vp, p = vp; i--; ++l_p, ++p)
> > + linux_to_bsd_dtv_property(l_p, p);
> > +
> > + error = copyout_map(td, &uvp, propsiz);
> > + if (error)
> > + goto out2;
> > + copyout(vp, (void *) uvp, propsiz);
> > +
> > + if ((error = fget(td, args->fd, &fp)) != 0) {
> > + (void) copyout_unmap(td, uvp, propsiz);
> > + goto out2;
> > + }
> > + vps.props = (void *) uvp;
> > + if ((args->cmd & 0xffff) == LINUX_FE_SET_PROPERTY)
> > + error = fo_ioctl(fp, FE_SET_PROPERTY, &vps, td->td_ucred, td);
> > + else
> > + error = fo_ioctl(fp, FE_GET_PROPERTY, &vps, td->td_ucred, td);
> > + if (error) {
> > + (void) copyout_unmap(td, uvp, propsiz);
> > + goto out;
> > + }
> > + error = copyin((void *) uvp, vp, propsiz);
> > + (void) copyout_unmap(td, uvp, propsiz);
> No need in space between cast and expression. Bigger issue is that I
> do not understand this fragment. You do copyout_map(), and then
> immediately call copyout_unmap() for the address range returned
> by copyout_map(), or am I missing something ?
>
The vm allocated by copyout_map() is only needed for the fo_ioctl()
call because the struct passed to FE_[GS]ET_PROPERTY describes an
array that the drivers then copyin() and (possibly) copyout()
themselves. So that array needs to be translated from/to the 32bit
Linux version to (possibly) 64bit on the host (linux_to_bsd_dtv_property),
and the 64bit version is larger so it doesn't fit in the original
place in the userland vm.
(One optimization here would be to only do this when the sizes
differ i.e. in the 32bit-on-64 case...)
Thanx!
Juergen
More information about the freebsd-hackers
mailing list