Re: [RFC PATCH] staging, android, ashmem: invalidate pin/unpin ioctl for private map
2012/12/3 Dan Carpenter : > On Mon, Dec 03, 2012 at 09:09:59AM +0900, JoonSoo Kim wrote: >> Hello, Dan. >> >> 2012/12/2 Dan Carpenter : >> > On Sat, Dec 01, 2012 at 02:45:57AM +0900, Joonsoo Kim wrote: >> >> @@ -614,21 +616,35 @@ static int ashmem_pin_unpin(struct ashmem_area >> >> *asma, unsigned long cmd, >> >> pgstart = pin.offset / PAGE_SIZE; >> >> pgend = pgstart + (pin.len / PAGE_SIZE) - 1; >> >> >> >> - mutex_lock(_mutex); >> >> + if (asma->shared_mapping) { >> >> + mutex_lock(_mutex); >> > >> > Wouldn't we need to hold the mutex while we check the >> > ->shared_mapping? >> >> I doesn't fully understand ashmem's lock semantic. >> Code for retrieving some value of asma instance doesn't hold the mutex, now. >> For example, in ashmem_ioctl(), asma->size, asma->prot_mask. >> And in ashmem_pin_unpin(), there is asma->file, asma->size which is >> retrieved without the mutex. >> According to this semantic, the mutex doesn't need for checking >> asma->shared_mapping. > > The ashmem_ioctl() is clearly racy. :P asma->size can be modified > and read at the same time. It's not an example to follow. Okay :) I will insert a code for holding the mutex in next spin. Thanks! -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] staging, android, ashmem: invalidate pin/unpin ioctl for private map
2012/12/3 Dan Carpenter dan.carpen...@oracle.com: On Mon, Dec 03, 2012 at 09:09:59AM +0900, JoonSoo Kim wrote: Hello, Dan. 2012/12/2 Dan Carpenter dan.carpen...@oracle.com: On Sat, Dec 01, 2012 at 02:45:57AM +0900, Joonsoo Kim wrote: @@ -614,21 +616,35 @@ static int ashmem_pin_unpin(struct ashmem_area *asma, unsigned long cmd, pgstart = pin.offset / PAGE_SIZE; pgend = pgstart + (pin.len / PAGE_SIZE) - 1; - mutex_lock(ashmem_mutex); + if (asma-shared_mapping) { + mutex_lock(ashmem_mutex); Wouldn't we need to hold the mutex while we check the -shared_mapping? I doesn't fully understand ashmem's lock semantic. Code for retrieving some value of asma instance doesn't hold the mutex, now. For example, in ashmem_ioctl(), asma-size, asma-prot_mask. And in ashmem_pin_unpin(), there is asma-file, asma-size which is retrieved without the mutex. According to this semantic, the mutex doesn't need for checking asma-shared_mapping. The ashmem_ioctl() is clearly racy. :P asma-size can be modified and read at the same time. It's not an example to follow. Okay :) I will insert a code for holding the mutex in next spin. Thanks! -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] staging, android, ashmem: invalidate pin/unpin ioctl for private map
On Mon, Dec 03, 2012 at 09:09:59AM +0900, JoonSoo Kim wrote: > Hello, Dan. > > 2012/12/2 Dan Carpenter : > > On Sat, Dec 01, 2012 at 02:45:57AM +0900, Joonsoo Kim wrote: > >> @@ -614,21 +616,35 @@ static int ashmem_pin_unpin(struct ashmem_area > >> *asma, unsigned long cmd, > >> pgstart = pin.offset / PAGE_SIZE; > >> pgend = pgstart + (pin.len / PAGE_SIZE) - 1; > >> > >> - mutex_lock(_mutex); > >> + if (asma->shared_mapping) { > >> + mutex_lock(_mutex); > > > > Wouldn't we need to hold the mutex while we check the > > ->shared_mapping? > > I doesn't fully understand ashmem's lock semantic. > Code for retrieving some value of asma instance doesn't hold the mutex, now. > For example, in ashmem_ioctl(), asma->size, asma->prot_mask. > And in ashmem_pin_unpin(), there is asma->file, asma->size which is > retrieved without the mutex. > According to this semantic, the mutex doesn't need for checking > asma->shared_mapping. The ashmem_ioctl() is clearly racy. :P asma->size can be modified and read at the same time. It's not an example to follow. regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] staging, android, ashmem: invalidate pin/unpin ioctl for private map
Hello, Dan. 2012/12/2 Dan Carpenter : > On Sat, Dec 01, 2012 at 02:45:57AM +0900, Joonsoo Kim wrote: >> @@ -614,21 +616,35 @@ static int ashmem_pin_unpin(struct ashmem_area *asma, >> unsigned long cmd, >> pgstart = pin.offset / PAGE_SIZE; >> pgend = pgstart + (pin.len / PAGE_SIZE) - 1; >> >> - mutex_lock(_mutex); >> + if (asma->shared_mapping) { >> + mutex_lock(_mutex); > > Wouldn't we need to hold the mutex while we check the > ->shared_mapping? I doesn't fully understand ashmem's lock semantic. Code for retrieving some value of asma instance doesn't hold the mutex, now. For example, in ashmem_ioctl(), asma->size, asma->prot_mask. And in ashmem_pin_unpin(), there is asma->file, asma->size which is retrieved without the mutex. According to this semantic, the mutex doesn't need for checking asma->shared_mapping. Thanks for review! -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] staging, android, ashmem: invalidate pin/unpin ioctl for private map
Hello, Dan. 2012/12/2 Dan Carpenter dan.carpen...@oracle.com: On Sat, Dec 01, 2012 at 02:45:57AM +0900, Joonsoo Kim wrote: @@ -614,21 +616,35 @@ static int ashmem_pin_unpin(struct ashmem_area *asma, unsigned long cmd, pgstart = pin.offset / PAGE_SIZE; pgend = pgstart + (pin.len / PAGE_SIZE) - 1; - mutex_lock(ashmem_mutex); + if (asma-shared_mapping) { + mutex_lock(ashmem_mutex); Wouldn't we need to hold the mutex while we check the -shared_mapping? I doesn't fully understand ashmem's lock semantic. Code for retrieving some value of asma instance doesn't hold the mutex, now. For example, in ashmem_ioctl(), asma-size, asma-prot_mask. And in ashmem_pin_unpin(), there is asma-file, asma-size which is retrieved without the mutex. According to this semantic, the mutex doesn't need for checking asma-shared_mapping. Thanks for review! -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] staging, android, ashmem: invalidate pin/unpin ioctl for private map
On Mon, Dec 03, 2012 at 09:09:59AM +0900, JoonSoo Kim wrote: Hello, Dan. 2012/12/2 Dan Carpenter dan.carpen...@oracle.com: On Sat, Dec 01, 2012 at 02:45:57AM +0900, Joonsoo Kim wrote: @@ -614,21 +616,35 @@ static int ashmem_pin_unpin(struct ashmem_area *asma, unsigned long cmd, pgstart = pin.offset / PAGE_SIZE; pgend = pgstart + (pin.len / PAGE_SIZE) - 1; - mutex_lock(ashmem_mutex); + if (asma-shared_mapping) { + mutex_lock(ashmem_mutex); Wouldn't we need to hold the mutex while we check the -shared_mapping? I doesn't fully understand ashmem's lock semantic. Code for retrieving some value of asma instance doesn't hold the mutex, now. For example, in ashmem_ioctl(), asma-size, asma-prot_mask. And in ashmem_pin_unpin(), there is asma-file, asma-size which is retrieved without the mutex. According to this semantic, the mutex doesn't need for checking asma-shared_mapping. The ashmem_ioctl() is clearly racy. :P asma-size can be modified and read at the same time. It's not an example to follow. regards, dan carpenter -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] staging, android, ashmem: invalidate pin/unpin ioctl for private map
On Sat, Dec 01, 2012 at 02:45:57AM +0900, Joonsoo Kim wrote: > @@ -614,21 +616,35 @@ static int ashmem_pin_unpin(struct ashmem_area *asma, > unsigned long cmd, > pgstart = pin.offset / PAGE_SIZE; > pgend = pgstart + (pin.len / PAGE_SIZE) - 1; > > - mutex_lock(_mutex); > + if (asma->shared_mapping) { > + mutex_lock(_mutex); Wouldn't we need to hold the mutex while we check the ->shared_mapping? regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] staging, android, ashmem: invalidate pin/unpin ioctl for private map
On Sat, Dec 01, 2012 at 02:45:57AM +0900, Joonsoo Kim wrote: @@ -614,21 +616,35 @@ static int ashmem_pin_unpin(struct ashmem_area *asma, unsigned long cmd, pgstart = pin.offset / PAGE_SIZE; pgend = pgstart + (pin.len / PAGE_SIZE) - 1; - mutex_lock(ashmem_mutex); + if (asma-shared_mapping) { + mutex_lock(ashmem_mutex); Wouldn't we need to hold the mutex while we check the -shared_mapping? regards, dan carpenter -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/