From owner-freebsd-hackers@FreeBSD.ORG Sat Jan 29 20:51:10 2011 Return-Path: Delivered-To: freebsd-hackers@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 65E72106566C; Sat, 29 Jan 2011 20:51:10 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from mail.zoral.com.ua (mx0.zoral.com.ua [91.193.166.200]) by mx1.freebsd.org (Postfix) with ESMTP id C67828FC08; Sat, 29 Jan 2011 20:51:09 +0000 (UTC) Received: from deviant.kiev.zoral.com.ua (root@deviant.kiev.zoral.com.ua [10.1.1.148]) by mail.zoral.com.ua (8.14.2/8.14.2) with ESMTP id p0TKp5iG009847 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Sat, 29 Jan 2011 22:51:05 +0200 (EET) (envelope-from kostikbel@gmail.com) Received: from deviant.kiev.zoral.com.ua (kostik@localhost [127.0.0.1]) by deviant.kiev.zoral.com.ua (8.14.4/8.14.4) with ESMTP id p0TKp5CP052051; Sat, 29 Jan 2011 22:51:05 +0200 (EET) (envelope-from kostikbel@gmail.com) Received: (from kostik@localhost) by deviant.kiev.zoral.com.ua (8.14.4/8.14.4/Submit) id p0TKp5SF052050; Sat, 29 Jan 2011 22:51:05 +0200 (EET) (envelope-from kostikbel@gmail.com) X-Authentication-Warning: deviant.kiev.zoral.com.ua: kostik set sender to kostikbel@gmail.com using -f Date: Sat, 29 Jan 2011 22:51:05 +0200 From: Kostik Belousov To: Juergen Lock Message-ID: <20110129205105.GI2518@deviant.kiev.zoral.com.ua> References: <20110129201000.GA10774@triton8.kn-bremen.de> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="DaUEczEJLvNJIL0g" Content-Disposition: inline In-Reply-To: <20110129201000.GA10774@triton8.kn-bremen.de> User-Agent: Mutt/1.4.2.3i X-Virus-Scanned: clamav-milter 0.95.2 at skuns.kiev.zoral.com.ua X-Virus-Status: Clean X-Spam-Status: No, score=-3.4 required=5.0 tests=ALL_TRUSTED,AWL,BAYES_00, DNS_FROM_OPENWHOIS autolearn=no version=3.2.5 X-Spam-Checker-Version: SpamAssassin 3.2.5 (2008-06-10) on skuns.kiev.zoral.com.ua Cc: freebsd-hackers@freebsd.org, freebsd-emulation@freebsd.org Subject: Re: Can vm_mmap()/vm_map_remove() be called with giant held? (linuxolator dvb patches) X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 29 Jan 2011 20:51:10 -0000 --DaUEczEJLvNJIL0g Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Sat, Jan 29, 2011 at 09:10:00PM +0100, Juergen Lock wrote: > Hi! >=20 > 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: >=20 > http://lists.freebsd.org/pipermail/freebsd-multimedia/2011-January/01157= 5.html >=20 > (patch also at: >=20 > http://people.freebsd.org/~nox/dvb/linux-dvb.patch >=20 > 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: >=20 > http://people.freebsd.org/~nox/dvb/linux-dvb-8.patch >=20 > ) >=20 > Thanx! > Juergen >=20 > Index: src/sys/compat/linux/linux_ioctl.c > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > 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 > #include > #include > +#include > +#include > +#include > + > +#include > +#include > +#include > +#include > =20 > #include > #include > @@ -83,6 +91,9 @@ __FBSDID("$FreeBSD: src/sys/compat/linux > #include > #include > =20 > +#include > +#include > + > CTASSERT(LINUX_IFNAMSIZ =3D=3D IFNAMSIZ); > =20 > 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; > =20 > @@ -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 =3D > { linux_ioctl_v4l, LINUX_IOCTL_VIDEO_MIN, LINUX_IOCTL_VIDEO_MAX }; > +static struct linux_ioctl_handler dvb_handler =3D > +{ linux_ioctl_dvb, LINUX_IOCTL_DVB_MIN, LINUX_IOCTL_DVB_MAX }; > static struct linux_ioctl_handler fbsd_usb =3D > { linux_ioctl_fbsd_usb, FBSD_LUSB_MIN, FBSD_LUSB_MAX }; > =20 > @@ -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); > =20 > struct handler_element > @@ -2989,6 +3004,251 @@ linux_ioctl_special(struct thread *td, s > } > =20 > /* > + * 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 =3D 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 =3D 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 =3D (vm_size_t) round_page(sz); > + > + error =3D 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 =3D &td->td_proc->p_vmspace->vm_map; > + size =3D (vm_size_t) round_page(sz); > + > + if (!vm_map_remove(map, addr, addr + size)) Better do if (vm_map_remove(...) !=3D KERN_SUCCESS) > + return (EINVAL); > + > + return (0); > +} > + > +static int > +linux_to_bsd_dtv_properties(struct l_dtv_properties *lvps, struct dtv_pr= operties *vps) > +{ Empty line between (empty) declaration section and executable statements. > + vps->num =3D lvps->num; > + vps->props =3D PTRIN(lvps->props); /* possible pointer size conversion = */ > + return (0); > +} > + > +static int > +linux_to_bsd_dtv_property(struct l_dtv_property *lvp, struct dtv_propert= y *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 =3D NULL; > + vp->result =3D 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 =3D PTROUT(NULL); > + lvp->result =3D 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 =3D NULL, *l_p; > + struct dtv_property *vp =3D 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) !=3D LINUX_IOC_VOID) > + return ENOIOCTL; > + args->cmd =3D (args->cmd & 0xffff) | IOC_VOID; > + break; > + > + case LINUX_DMX_REMOVE_PID: > + /* overlaps with LINUX_NET_ADD_IF */ > + if ((args->cmd & IOC_DIRMASK) =3D=3D 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 =3D (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 =3D (args->cmd & ~IOC_DIRMASK) | IOC_OUT; > + break; > + > + case LINUX_DMX_GET_STC: > + case LINUX_NET_GET_IF: > + net_add_if: > + args->cmd =3D (args->cmd & ~IOC_DIRMASK) | IOC_INOUT; > + break; > + > + case LINUX_FE_SET_PROPERTY: > + case LINUX_FE_GET_PROPERTY: > + error =3D copyin((void *) args->arg, &l_vps, sizeof(l_vps)); > + if (error) > + return (error); > + linux_to_bsd_dtv_properties(&l_vps, &vps); > + if ((vps.num =3D=3D 0) || vps.num > DTV_IOCTL_MAX_MSGS) > + return EINVAL; > + > + l_propsiz =3D vps.num * sizeof(*l_vp); > + propsiz =3D vps.num * sizeof(*vp); > + if (((l_vp =3D malloc(l_propsiz, M_LINUX, M_WAITOK)) =3D=3D NULL) || > + (vp =3D malloc(propsiz, M_LINUX, M_WAITOK)) =3D=3D NULL) { Malloc(M_WAITOK) is guaranteed to never return NULL, you do not need the checks. > + error =3D ENOMEM; > + goto out2; > + } > + error =3D copyin((void *) vps.props, l_vp, l_propsiz); > + if (error) > + goto out2; > + for (i =3D vps.num, l_p =3D l_vp, p =3D vp; i--; ++l_p, ++p) > + linux_to_bsd_dtv_property(l_p, p); > + > + error =3D copyout_map(td, &uvp, propsiz); > + if (error) > + goto out2; > + copyout(vp, (void *) uvp, propsiz); > + > + if ((error =3D fget(td, args->fd, &fp)) !=3D 0) { > + (void) copyout_unmap(td, uvp, propsiz); > + goto out2; > + } > + vps.props =3D (void *) uvp; > + if ((args->cmd & 0xffff) =3D=3D LINUX_FE_SET_PROPERTY) > + error =3D fo_ioctl(fp, FE_SET_PROPERTY, &vps, td->td_ucred, td); > + else > + error =3D fo_ioctl(fp, FE_GET_PROPERTY, &vps, td->td_ucred, td); > + if (error) { > + (void) copyout_unmap(td, uvp, propsiz); > + goto out; > + } > + error =3D 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] --DaUEczEJLvNJIL0g Content-Type: application/pgp-signature Content-Disposition: inline -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (FreeBSD) iEYEARECAAYFAk1EfbcACgkQC3+MBN1Mb4hvVgCgsrpJXvp28a9tqjPRfbXwysl3 CmkAn2hYTdD44azUhI6WfRHWhaOnrfKS =0ED8 -----END PGP SIGNATURE----- --DaUEczEJLvNJIL0g--