Hide Forgot
megaraid driver 1b08b and 1e08 (functionally identical) both have this same problem which caused Alan to reject the driver update. AMI needs Alan's help to resolve this issue properly. Rejected. This is completely unsafe #if LINUX_VERSION_CODE > 0x020024 /* * usage of the function copy from user is used in case of data more than * 4KB. This is used only with adapters which supports more than 8 logical * drives. This feature is disabled on kernels earlier or same as 2.0.36 * as the uaccess.h file is not available with those kernels. */ if (SCpnt->cmnd[0] == IOCTL_CMD_NEW) { /* use external data area for large xfers */ /* If cmnd[0] is set to IOCTL_CMD_NEW then * * cmnd[4..7] = external user buffer * * cmnd[8..11] = length of buffer * * */ u32 xfer_size = *((u32 *)&SCpnt->cmnd[8]); user_area = *((char **)&SCpnt->cmnd[4]); if (verify_area(VERIFY_READ, user_area, xfer_size)) { printk("megaraid: Got bad user address.\n"); SCpnt->result = (DID_ERROR << 16); callDone (SCpnt); return NULL; } mega_build_user_sg(user_area, xfer_size, pScb, mbox); } #endif You cannot go around peeking into unlocked user pages. Nor can you assume they are even reachable by PCI. You can scatter gather fine but use get_free_page() and copy the data. Alan
This defect is considered MUST-FIX for Winston Release-Candidate #1 ... this bug needs some serious attention (based on Alan's comments), Doug. Please do what you can to take care of this defect ASAP. I don't want to have another "post-release AMI Mega-RAID" issue like we did in Zoot. :-)
I have talked with Peter about doing it the way it was done before and I think we are both happy on that and the nasty hack removed. Check with PeterJ
Doug's second megaraid patch dereferences a NULL pointer somewhere when the driver loads. He thought it might not be 100% ready, but more a proof-of- concept. I'm afraid I haven't had time to debug it properly, so if Michael or PeterJ can, that's great.
Created attachment 2075 [details] megaraid.patch
Doug's comments on making the above patch: OK, attached is a new version of the patch I sent last night. It fixes the fact that my first patch was really begging me not to leak 32K of kmalloc'ed RAM with every ioctl (doh!!) and it also moves to per Scb semaphores so that multiple ioctl commands won't step on each other. Finally, I'm not a semaphore expert, so should I be adding a wait queue to the semaphore init, or does the kernel use a generic one in this case? I think it just uses a generic one and the right spot will get woken up when the up() happens, but I could be wrong. Anyway, the process I have in there should be sound. The main thing is that you can't hold the io_request_lock or the driver lock or any other lock for that matter during either the down() or the copy_to_user(), the rest of the time you should be locked.
Giving this one back to you, Doug! Thanks...
AMI found the NULL pointer dereference bug that Doug had left in there. The rest of it looks great! AMI and Dell are testing, and will resubmit ASAP. Peter, when you're confident, can you release a 1.09 driver? -Matt
ASAP is putting it very lightly. Peter, if you are not confident enough to release a 1.09 driver, could you please at least send the fix for the NULL pointer deref for us to drop in for initial tests -- I need it in a few *hours* to make RC1.
Created attachment 2234 [details] megaraid-1.09.tgz
The above megaraid-1.09.tgz is exactly the 1b0b driver, + Doug's patch, + Manoj's fix to Doug's patch. I'm sending this now because time is short. I'm not precluding PeterJ from sending a "better" version. We've tested this driver somewhat, and it certainly solves the NULL pointer dereference issue.
Thanks for your quick response, Matt. I'm in the process of integrating the patch now. I expect to give you a kernel tomorrow for testing, and it will have this version of the driver in it.
OK, Matt, there is now a 2.2.16-19.1 kernel in your private directory on gribble. Please test. Peter, if you want a copy, I can give one to you, or you can get it from Matt, whichever works best.
Since the driver is integrated, I'm closing this. Matt, I'm a bit confused about the megamgr program bug that you reported earlier; want to open a new bug report about that bug if it is a real bug?
I don't know if it's a real bug yet or not, but I'll open a new issue if/when I can prove either way.