Skip to contentSkip to conversation

Important - MM panic in 2.1.109 [PATCH + Oops]

DISCUSSION Applied Stephen C. Tweedie 2 participants 2 messages Jul 23, 1998 2 messages not in the archive Memory management

lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

A message that is not in the archive (sent off-list or not yet received): [email protected]

Stephen C. Tweedie

author
#2 lore raw

Hi,

Quoted 5 lines · Michael L. Galbraith
Quoted 4 lines · Itai Nahshon

1. I sent a similar report some time ago.. the panic still happens. The problem is when unmapping the last part (but not all) of a segment acquired by shmget + shmat. Ref count goes down to 0 and then the segment is reused!!

Definitely reproducible, thanks.

Quoted 10 lines

Hi Folks,

I've attached an oops for _real_ hackers to examine. Patch follows. It should illuminate the problem, and at least does stop the oops.

ipc/shm.c renamed from ipc/shm.c.org +0 -0
@@ -626,7 +626,12 @@
 	remove_attach(shp,shmd);  /* remove from shp->attaches */
 shp-> shm_lpid = current->pid;

ipc/shm.c line 636

Quoted 10 lines
ipc/shm.c renamed from ipc/shm.c.org +6 -1
 shp-> shm_dtime = CURRENT_TIME;
-	if (--shp->shm_nattch <= 0 && shp->shm_perm.mode & SHM_DEST)
+	/* FIXME: If vm_end = vm_start, we are doing unmap_fixup().
+	 * This is very fragile and stupid.  It's also the best I
+	 * could come up with without special casing unmap_fixup().
+	 */
+	if (--shp->shm_nattch <= 0 && shp->shm_perm.mode & SHM_DEST
+			&& shmd->vm_end != shmd->vm_start)
 		killseg (id);
 }

This fixes the problem right enough.

However, there's an alternative fix in mm/mmap.c:

----------------------------------------------------------------

mmap.c Thu Jul 23 15:39:50 1998 renamed from mmap.c~ Wed Jul 22 14:48:04 1998 +0 -10
diff -u mm/mmap.c~ mm/mmap.c
@@ -421,16 +421,6 @@
                insert_vm_struct(current->mm, mpnt);
        }
 
-       /* Close the current area ... */
-       if (area->vm_ops && area->vm_ops->close) {
-               end = area->vm_end; /* save new end */
-               area->vm_end = area->vm_start;
-               area->vm_ops->close(area);
-               area->vm_end = end;
-       }
-       /* ... then reopen and reinsert. */
-       if (area->vm_ops && area->vm_ops->open)
-               area->vm_ops->open(area);
        insert_vm_struct(current->mm, area);
        return 1;
 }

----------------------------------------------------------------

This seems to be a far less messy way to deal with the underlying bug, which arises due to the close then reopen of a vma if we unmap from one end. *Any* vm type which refcounts its objects will be hit by this behaviour in unmap_fixup(), which lets the underlying mapped object's refcount go to zero only to be brought back up to one via the same vma.

Currently the only user of vm->open and ->close is shm, and it is only used as a reference counting mechanism. If the reference count is not being modified, then why should we call close/open at all, especially if we don't even bother to supply the correct vm_start parameter to the close call?

The oops is definitely eliminated by the second patch, although the first one looks reasonable too.

--Stephen
--
This is a majordomo managed list. To unsubscribe, send a message with the body 'unsubscribe linux-mm me@address' to: [email protected]

Applied
#3 lore raw
Quoted 2 lines · Stephen C. Tweedie

However, there's an alternative fix in mm/mmap.c:

[ patch removed ]

Applied. I always prefer to fix bug by removing code rather than adding it ;)

Linus

--
This is a majordomo managed list. To unsubscribe, send a message with the body 'unsubscribe linux-mm me@address' to: [email protected]