Date: Wed, 10 Feb 2010 20:20:37 -0800 From: Marcel Moolenaar <xcllnt@mac.com> To: Kostik Belousov <kostikbel@gmail.com> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r203696 - in head: lib/libc/sys sys/kern sys/sys Message-ID: <FBE6FB42-0DE8-439C-9182-EFC3A81CABCF@mac.com> In-Reply-To: <20100210091522.GW9991@deviant.kiev.zoral.com.ua> References: <201002090552.o195qZcD074581@svn.freebsd.org> <20100209095722.GQ9991@deviant.kiev.zoral.com.ua> <65DCE552-7EFD-48F2-85A4-EA0F1F0638EE@mac.com> <20100209184043.GV9991@deviant.kiev.zoral.com.ua> <896B58E6-12EA-48AB-86C2-5BA9F0C59512@mac.com> <86989446-64EF-411F-8E25-173DB6AEE10B@mac.com> <20100210091522.GW9991@deviant.kiev.zoral.com.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
[-- Attachment #1 --]
On Feb 10, 2010, at 1:15 AM, Kostik Belousov wrote:
>
> Vnode locks are before vm map locks in global lock order. vn_fullpath()
> may need to lock vnodes to call VOP_VPTOCNP(). I think you should (and
> can) drop both vm map lock and vmspace reference much earlier.
>
> Would it be cleaner to use explicitely sized types for compat32
> structure members ?
I don't know. I prefer to keep them identical for as much as
that's possible.
> Comparing ptrace_vm_entry with kinfo_vmentry, I think that it might
> be good idea to add fsid and inode number to ptrace_vm_entry, to
> give at least some information when vn_fullpath failed.
How about the attached new path (includes man page as well)?
--
Marcel Moolenaar
xcllnt@mac.com
[-- Attachment #2 --]
Index: lib/libc/sys/ptrace.2
===================================================================
--- lib/libc/sys/ptrace.2 (revision 203696)
+++ lib/libc/sys/ptrace.2 (working copy)
@@ -343,23 +343,30 @@
which is defined as follows:
.Bd -literal
struct ptrace_vm_entry {
- void *pve_cookie;
- u_long pve_start;
- u_long pve_end;
- u_long pve_offset;
- u_int pve_prot;
- u_int pve_pathlen;
- char *pve_path;
+ int pve_entry;
+ int pve_timestamp;
+ u_long pve_start;
+ u_long pve_end;
+ u_long pve_offset;
+ u_int pve_prot;
+ u_int pve_pathlen;
+ long pve_fileid;
+ uint32_t pve_fsid;
+ char *pve_path;
};
.Ed
.Pp
The first entry is returned by setting
-.Va pve_cookie
-to
-.Dv NULL .
+.Va pve_entry
+to zero.
Subsequent entries are returned by leaving
-.Va pve_cookie
+.Va pve_entry
unmodified from the value returned by previous requests.
+The
+.Va pve_timestamp
+field can be used to detect changes to the VM map while iterating over the
+entries.
+The tracing process can then take appropriate action, such as restarting.
By setting
.Va pve_pathlen
to a non-zero value on entry, the pathname of the backing object is returned
@@ -434,7 +441,8 @@
.It
.Dv PT_VM_ENTRY
was given an invalid value for
-.Fa pve_cookie .
+.Fa pve_entry .
+This can also be caused by changes to the VM map of the process.
.El
.It Bq Er EBUSY
.Bl -bullet -compact
Index: sys/kern/sys_process.c
===================================================================
--- sys/kern/sys_process.c (revision 203724)
+++ sys/kern/sys_process.c (working copy)
@@ -75,12 +75,15 @@
};
struct ptrace_vm_entry32 {
- uint32_t pve_cookie;
+ int pve_entry;
+ int pve_timestamp;
uint32_t pve_start;
uint32_t pve_end;
uint32_t pve_offset;
u_int pve_prot;
u_int pve_pathlen;
+ int32_t pve_fileid;
+ u_int pve_fsid;
uint32_t pve_path;
};
@@ -360,88 +363,141 @@
static int
ptrace_vm_entry(struct thread *td, struct proc *p, struct ptrace_vm_entry *pve)
{
+ struct vattr vattr;
vm_map_t map;
vm_map_entry_t entry;
vm_object_t obj, tobj, lobj;
+ struct vmspace *vm;
struct vnode *vp;
char *freepath, *fullpath;
u_int pathlen;
- int error, vfslocked;
+ int error, index, vfslocked;
- map = &p->p_vmspace->vm_map;
- entry = map->header.next;
- if (pve->pve_cookie != NULL) {
- while (entry != &map->header && entry != pve->pve_cookie)
+ error = 0;
+ obj = NULL;
+
+ vm = vmspace_acquire_ref(p);
+ map = &vm->vm_map;
+ vm_map_lock_read(map);
+
+ do {
+ entry = map->header.next;
+ index = 0;
+ while (index < pve->pve_entry && entry != &map->header) {
entry = entry->next;
- if (entry != pve->pve_cookie)
- return (EINVAL);
- entry = entry->next;
- }
- while (entry != &map->header && (entry->eflags & MAP_ENTRY_IS_SUB_MAP))
- entry = entry->next;
- if (entry == &map->header)
- return (ENOENT);
+ index++;
+ }
+ if (index != pve->pve_entry) {
+ error = EINVAL;
+ break;
+ }
+ while (entry != &map->header &&
+ (entry->eflags & MAP_ENTRY_IS_SUB_MAP) != 0) {
+ entry = entry->next;
+ index++;
+ }
+ if (entry == &map->header) {
+ error = ENOENT;
+ break;
+ }
- /* We got an entry. */
- pve->pve_cookie = entry;
- pve->pve_start = entry->start;
- pve->pve_end = entry->end - 1;
- pve->pve_offset = entry->offset;
- pve->pve_prot = entry->protection;
+ /* We got an entry. */
+ pve->pve_entry = index + 1;
+ pve->pve_timestamp = map->timestamp;
+ pve->pve_start = entry->start;
+ pve->pve_end = entry->end - 1;
+ pve->pve_offset = entry->offset;
+ pve->pve_prot = entry->protection;
- /* Backing object's path needed? */
- if (pve->pve_pathlen == 0)
- return (0);
+ /* Backing object's path needed? */
+ if (pve->pve_pathlen == 0)
+ break;
- pathlen = pve->pve_pathlen;
- pve->pve_pathlen = 0;
+ pathlen = pve->pve_pathlen;
+ pve->pve_pathlen = 0;
- obj = entry->object.vm_object;
- if (obj == NULL)
- return (0);
+ obj = entry->object.vm_object;
+ if (obj != NULL)
+ VM_OBJECT_LOCK(obj);
+ } while (0);
- VM_OBJECT_LOCK(obj);
- for (lobj = tobj = obj; tobj; tobj = tobj->backing_object) {
- if (tobj != obj)
- VM_OBJECT_LOCK(tobj);
- if (lobj != obj)
- VM_OBJECT_UNLOCK(lobj);
- lobj = tobj;
- pve->pve_offset += tobj->backing_object_offset;
- }
- if (lobj != NULL) {
+ vm_map_unlock_read(map);
+ vmspace_free(vm);
+
+ if (error == 0 && obj != NULL) {
+ lobj = obj;
+ for (tobj = obj; tobj != NULL; tobj = tobj->backing_object) {
+ if (tobj != obj)
+ VM_OBJECT_LOCK(tobj);
+ if (lobj != obj)
+ VM_OBJECT_UNLOCK(lobj);
+ lobj = tobj;
+ pve->pve_offset += tobj->backing_object_offset;
+ }
vp = (lobj->type == OBJT_VNODE) ? lobj->handle : NULL;
if (vp != NULL)
vref(vp);
if (lobj != obj)
VM_OBJECT_UNLOCK(lobj);
VM_OBJECT_UNLOCK(obj);
- } else
- vp = NULL;
- if (vp == NULL)
- return (0);
+ if (vp != NULL) {
+ freepath = NULL;
+ fullpath = NULL;
+ vn_fullpath(td, vp, &fullpath, &freepath);
+ vfslocked = VFS_LOCK_GIANT(vp->v_mount);
+ vn_lock(vp, LK_SHARED | LK_RETRY);
+ if (VOP_GETATTR(vp, &vattr, td->td_ucred) == 0) {
+ pve->pve_fileid = vattr.va_fileid;
+ pve->pve_fsid = vattr.va_fsid;
+ }
+ vput(vp);
+ VFS_UNLOCK_GIANT(vfslocked);
- freepath = NULL;
- fullpath = NULL;
- vn_fullpath(td, vp, &fullpath, &freepath);
- vfslocked = VFS_LOCK_GIANT(vp->v_mount);
- vrele(vp);
- VFS_UNLOCK_GIANT(vfslocked);
+ if (fullpath != NULL) {
+ pve->pve_pathlen = strlen(fullpath) + 1;
+ if (pve->pve_pathlen <= pathlen) {
+ error = copyout(fullpath, pve->pve_path,
+ pve->pve_pathlen);
+ } else
+ error = ENAMETOOLONG;
+ }
+ if (freepath != NULL)
+ free(freepath, M_TEMP);
+ }
+ }
- error = 0;
- if (fullpath != NULL) {
- pve->pve_pathlen = strlen(fullpath) + 1;
- if (pve->pve_pathlen <= pathlen) {
- error = copyout(fullpath, pve->pve_path,
- pve->pve_pathlen);
- } else
- error = ENAMETOOLONG;
+ return (error);
+}
+
+#ifdef COMPAT_IA32
+static int
+ptrace_vm_entry32(struct thread *td, struct proc *p,
+ struct ptrace_vm_entry32 *pve32)
+{
+ struct ptrace_vm_entry pve;
+ int error;
+
+ pve.pve_entry = pve32->pve_entry;
+ pve.pve_pathlen = pve32->pve_pathlen;
+ pve.pve_path = (void *)(uintptr_t)pve32->pve_path;
+
+ error = ptrace_vm_entry(td, p, &pve);
+ if (error == 0) {
+ pve32->pve_entry = pve.pve_entry;
+ pve32->pve_timestamp = pve.pve_timestamp;
+ pve32->pve_start = pve.pve_start;
+ pve32->pve_end = pve.pve_end;
+ pve32->pve_offset = pve.pve_offset;
+ pve32->pve_prot = pve.pve_prot;
+ pve32->pve_fileid = pve.pve_fileid;
+ pve32->pve_fsid = pve.pve_fsid;
}
- if (freepath != NULL)
- free(freepath, M_TEMP);
+
+ pve32->pve_pathlen = pve.pve_pathlen;
return (error);
}
+#endif /* COMPAT_IA32 */
/*
* Process debugging system call.
@@ -1087,14 +1143,12 @@
break;
case PT_VM_ENTRY:
+ PROC_UNLOCK(p);
#ifdef COMPAT_IA32
- /* XXX to be implemented. */
- if (wrap32) {
- error = EDOOFUS;
- break;
- }
+ if (wrap32)
+ error = ptrace_vm_entry32(td, p, addr);
+ else
#endif
- PROC_UNLOCK(p);
error = ptrace_vm_entry(td, p, addr);
PROC_LOCK(p);
break;
Index: sys/sys/ptrace.h
===================================================================
--- sys/sys/ptrace.h (revision 203724)
+++ sys/sys/ptrace.h (working copy)
@@ -104,13 +104,16 @@
/* Argument structure for PT_VM_ENTRY. */
struct ptrace_vm_entry {
- void *pve_cookie; /* Token used to iterate. */
- u_long pve_start; /* Start VA of range. */
- u_long pve_end; /* End VA of range (incl). */
- u_long pve_offset; /* Offset in backing object. */
- u_int pve_prot; /* Protection of memory range. */
- u_int pve_pathlen; /* Size of path. */
- char *pve_path; /* Path name of object. */
+ int pve_entry; /* Entry number used for iteration. */
+ int pve_timestamp; /* Generation number of VM map. */
+ u_long pve_start; /* Start VA of range. */
+ u_long pve_end; /* End VA of range (incl). */
+ u_long pve_offset; /* Offset in backing object. */
+ u_int pve_prot; /* Protection of memory range. */
+ u_int pve_pathlen; /* Size of path. */
+ long pve_fileid; /* File ID. */
+ uint32_t pve_fsid; /* File system ID. */
+ char *pve_path; /* Path name of object. */
};
#ifdef _KERNEL
[-- Attachment #3 --]
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?FBE6FB42-0DE8-439C-9182-EFC3A81CABCF>