On 02/28/2013 03:14 AM, Chris Wilson wrote: > On Wed, Feb 27, 2013 at 05:15:08PM -0800, Ian Romanick wrote: >> On 02/27/2013 01:13 AM, Chris Wilson wrote: >>> On Tue, Feb 26, 2013 at 05:47:12PM -0800, Ian Romanick wrote: >>>> On 02/26/2013 03:05 AM, Mika Kuoppala wrote: >>>>> For arb-robustness, every context needs to have it's own >>>>> reset state tracking. Default context will be handled in a identical >>>>> way as the no-context case in further down in the patch set. >>>>> For no-context case, the reset state will be stored in >>>>> the file_priv part. >>>>> >>>>> v2: handle default context inside get_reset_state >>>> >>>> This isn't the interface we want. I already sent you the patches >>>> for Mesa, and you seem to have completely ignored them. Moreover, >>>> this interface provides no mechanism to query for its existence >>>> (other than relying on the kernel version), and no method to >>>> deprecate it. >>> >>> It's existence, and the existence of any successor, is the only test you >>> need to check for the interface. Testing the flag field up front also >>> lets you know if individual flags are known prior to use. >>> >>>> Based on e-mail discussions, I think danvet agrees with me here. >>>> >>>> Putting guilty / innocent counting in kernel puts policy decisions >>>> in the kernel that belong with the user space API that implements >>>> them. Putting these choices in the kernel significantly decreases >>>> how "future proof" the interface is. Since any kernel/user >>>> interface has to be kept forever, this is just asking for >>>> maintenance trouble down the road. >>> >>> I think you have the policy standpoint reversed. >> >> Can you elaborate? I've been out of the kernel loop for a long >> time, but I always thought policy and mechanism were supposed to be >> separated. In the case of drivers with a user-mode component, the >> mechanism was in the kernel (where else could it be?), and policy >> decisions were in user-mode. This is often at odds with keeping the >> interfaces between kernel and user thin, so that may be where my >> misunderstanding is. > > Right, the idea is to keep policy out of the kernel. I disagree that > your suggestion succeeds in doing this. I was trying to put the mechanism for determining what things happened in the kernel and the policy for interpreting what those things mean in user-mode. Ignoring some bugs / misimplementation in my patch (which I try to address below), I thought I was doing that. User-mode gets some information about the state it was in when the reset occurred. It then uses that state to decide the value to return to the application for glGetGraphicsResetStatusARB. After reading the bits below where I talk about other problems in the kernel code I posted, maybe you can address specific ways my proposal fails. I want any glaring interface deficiencies fixed before any code goes in the kernel... I fully understand the pain, on both sides, of shipping a kernel interface only to have to replace it in a year. > [snip to details] > >> There are two requirements in the ARB_robustness extension (and >> additional layered extensions) that I believe cause problems with >> all of the proposed counting interfaces. >> >> 1. GL contexts that are not affected by a reset (i.e., didn't lose >> any objects, didn't lose any rendering, etc.) should not observe it. >> This is the big one that all the browser vendors want. If you have >> 5 tabs open, a reset caused by a malicious WebGL app in one tab >> shouldn't, if possible, force them to rebuild all of their state for >> all the other tabs. >> >> 2. After a GL context observes a reset, it is garbage. It must be >> destroyed and a new one created. This means we only need to know >> that a reset of any variety affected the context. > > For me observes implies the ability for the context to inspect global > system state, whereas I think you mean if the context and its associated > state is affected by a reset. I had thought about explicitly defining what I meant by a couple terms in my previous e-mail, but it was already getting quite long. I guess I should have. :( Here's what I meant: A. "Affect the context" means the reset causes the context to lose some data. It may be rendering that was queued (so framebuffer data is lost), it may be the contents of a buffer, or it may be something else. Either way, the state of some data is not what the application expects it to be because of the reset. B. "Observe the reset" means the GL context gets some data to know that the reset happened. From the application point of view, this means glGetGraphicsResetStatusARB returns a value other than GL_NO_ERROR. What I was trying the describe in point 1 in my previous message (above) is that B should occur if and only if A occurs. Once B occurs, the application has to create a new context... recompile all of its shaders, reload all of its textures, reload all of its vertex data, etc. We don't want to make apps do that any more often than absolutely necessary. >> In addition, I'm concerned about having one GL context be able to >> observe, in any manner, resets that did not affect it. Given that >> people have figured out how to get encryption keys by observing >> cache timings, it's not impossible for a malicious program to use >> information about reset counts to do something. Leaving a potential >> gap like this in an extension that's used to improved security >> seems... like we're just asking for a Phoronix article. We don't >> have any requirement to expose this information, so I don't think we >> should. > > So we should not do minor+major pagefault tracking either? I only > suggested that you could read the global state because I thought you > implied you wanted it. From the display server POV, global state is the > most useful as I am more interested in whether I can reliably use the GPU > and prevent a DoS from a misbehaving set of clients. In the X model, this would be easy to solve. We would make the total-number-of-resets query only available to be DRM master. As we move away from that model, it becomes more difficult. It seems that the usage is different enough that having separate queries for the global reset count and the per-context resets feels right. That still leaves the potential information leakage issue, and I'm honestly not sure what the right answer is there. Perhaps we could add that query after we've gotten some feedback from an actual security expert. There may be other options that we haven't considered yet. Maybe something like advertising the GPU reset "load average." Dunno. >> We also don't have any requirement to expose this functionality on >> anything pre-Sandy Bridge. We may want, at some point, to expose it >> on Ironlake. Based on that, it seems reasonable to tie the >> availability of reset notifications to hardware contexts. Since we >> won't test it, Mesa certainly isn't going to expose it on 8xx or >> 915. > >>From a design standpoint hw contexts are just one function of the context > object and not the reverse. Ben has been rightfully arguing that we need > contexts in the kernel for far more than supporting logical hw contexts. That's a fair point, and I also tend to agree with Ben. I think it may also be an orthogonal issue. As things currently stand (at least as far as I can tell) kernel contexts are tied to logical hw contexts. Since we don't have a requirement for reset query support on older GPUs, it doesn't seem to make sense to put extra code in the kernel, that nobody will use, to do it. If at some point the linkage between kernel contexts and hw contexts, and we can get a bunch of this mostly for free, then it makes sense to add the support then. Every single time I've written "future" code like that, it has either been broken or it turns out to not be what I need when the future arrives. I know I'm not the only one with that experience. >> Based on this, a simplified interface became obvious: for each >> hardware context, track a mask of kinds of resets that have affected >> that context. I defined three bits in the mask: > > The problem is that you added an atomic-read-and-reset into the ioctl. I > objected to that policy when Mika presented it earlier as it prevents > concurrent accesses and so imposes an explicit usage model. I may have mentioned that my kernel patch was very early work and should be taken with a grain of salt. :) You probably noticed that the patch was generated by diff and not by git-format-patch. I hadn't even committed any of that to my local tree. Given its shabby state, I was reluctant to even send it out. However, if I'm going to be critical of another person's work, I have a responsibility to be fully open. I too came to the conclusion that the atomic-read-and-reset part was a bad idea... I had forgotten that I even coded it that way, or I would have mentioned that in my previous message. The other thing in my kernel patch that I think is a bad idea (as you note below) is that processes can query the reset state of HW contexts associated with different fds. That coupled with the atomic-read-and-reset is completely disastrous. I think this should be tightened up for all the reasons we've both mentioned. >> 1. The hw context had a batch executing when the reset occurred. >> >> 2. The hw context had a batch pending when the reset occurred. >> Hopefully in the future we could make most occurrences of this go >> away by re-queuing the batch. It may also be possible to do this in >> user-mode, but that may be more difficult. >> >> 3. "Other." >> >> There's also the potential to add other bits to communicate >> information about lost buffers, textures, etc. This could be used >> to create a layered extension that lets applications transfer data >> from the dead context to the new context. Browsers may not be too >> interested in this, but I could imagine compositors or other kinds >> of applications benefiting. We may never implement that, but it's >> easier to communicate this through a set of flags than through a set >> of counts. >> >> The guilty / innocent counts in this patch series lose information. > > Disagree. The flags represent a lose of information, as you explained > earlier. With the counts in this series, user-mode can't tell the difference between a reset that didn't affect the context and one that did. Once a GL context observes a reset (i.e., when glGetGraphicsResetStatusARB returns non-GL_NO_ERROR), it is dead. The only numbers that matter in that case are zero and not-zero. If we assume that applications using this interface will query resets on a periodic basis, and both the major Linux browser vendors have told me that they do, the count will either be zero or some very small number. Given that, there's very little information lost, and the information that is lost is not important to the consumers of that information. Now, this isn't the case with the display manager use case. That case clearly wants counts, and it wants counts over time. Like I said above, I think this is a different enough use case that having a separate mechanism may be the right approach. >> We can probably reconstruct the current flags from the counts, but >> we would have to do some refactoring (or additional counting) to get >> other flags in the future. All of this would make the kernel code >> more complex. Why reconstruct the data you want when you could just >> track that data in the first place? > > Because you are constructing policy decision based on raw information. > >> Since the functionality in not available on all hardware, I also >> added a query to determine the availability. This has the added >> benefit that, should this interface prove to be insufficient in the >> future, we could deprecate it by having the query always return >> false. > > That offers no more information than just probing the ioctl. There are a couple other bits of kernel functionality that Mesa queries using this method before using (e.g., GEN7 SOL reset). Since this was, as far as I could tell, similarly querying kernel support for some hardware functionality, I assumed it should have a similar sort of query. In the future, what metric should I use to determine which is which? Also, what does "probing the ioctl" mean exactly? Just call it and see whether you get ENOSYS? Doesn't that run afoul of the requirement to never remove interfaces? >> All software involved has to be able the handle the case >> where the interface is not available, so I don't think this should >> run awry of the rules about kernel/user interface breakage. Please >> correct me if I'm wrong. > > [snip patch locations] > > The biggest change you made was to reduce the counts to a set of flags, > to make it harder for an attacker to analyse the reset frequency. > Instead that attacker can just poll the flags and coarsely reconstruct > the counts... If that is their goal. Instead it transfers userspace > policy into the kernel, which is what we were seeking to avoid, right? > -Chris
