Can vm_mmap()/vm_map_remove() be called with giant held?
(linuxolator dvb patches)
Kostik Belousov
kostikbel at gmail.com
Sat Jan 29 20:51:10 UTC 2011
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.
> 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 ?
> 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].
> + 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 ?
> +
> + /* 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.
> + 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.
> + 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 ?
[snip]
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 196 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/freebsd-hackers/attachments/20110129/5c55b6d7/attachment.pgp
More information about the freebsd-hackers
mailing list